* [PATCH 1/2] dt-bindings: iio: Add TI OPT4048 color sensor bindings
@ 2024-11-30 17:42 Paul Kocialkowski
2024-11-30 17:42 ` [PATCH 2/2] iio: light: Add support for the TI OPT4048 color sensor Paul Kocialkowski
2024-12-02 8:41 ` [PATCH 1/2] dt-bindings: iio: Add TI OPT4048 color sensor bindings Krzysztof Kozlowski
0 siblings, 2 replies; 10+ messages in thread
From: Paul Kocialkowski @ 2024-11-30 17:42 UTC (permalink / raw)
To: linux-iio, devicetree, linux-kernel
Cc: Jonathan Cameron, Lars-Peter Clausen, Rob Herring,
Krzysztof Kozlowski, Conor Dooley, Paul Kocialkowski
The Texas Instruments OPT4048 is a XYZ tristimulus color sensor.
It requires a VDD power supply and can optionally support an interrupt.
Signed-off-by: Paul Kocialkowski <paulk@sys-base.io>
---
.../bindings/iio/light/ti,opt4048.yaml | 54 +++++++++++++++++++
1 file changed, 54 insertions(+)
create mode 100644 Documentation/devicetree/bindings/iio/light/ti,opt4048.yaml
diff --git a/Documentation/devicetree/bindings/iio/light/ti,opt4048.yaml b/Documentation/devicetree/bindings/iio/light/ti,opt4048.yaml
new file mode 100644
index 000000000000..e2b7472ab588
--- /dev/null
+++ b/Documentation/devicetree/bindings/iio/light/ti,opt4048.yaml
@@ -0,0 +1,54 @@
+# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/iio/light/ti,opt4048.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Texas Instruments OPT4048 XYZ tristimulus color sensor
+
+maintainers:
+ - Paul Kocialkowski <paulk@sys-base.io>
+
+description: |
+ The device supports both interrupt-driven and interrupt-less operation,
+ depending on whether an interrupt property is present in the device-tree.
+
+properties:
+ compatible:
+ const: ti,opt4048
+
+ reg:
+ maxItems: 1
+
+ interrupts:
+ maxItems: 1
+ description: |
+ The interrupt is detected on a falling edge, with a low level asserted
+ for 1 us. It might be missed because of hardware interrupt debouncing
+ mechanisms due to this short time.
+
+ vdd-supply: true
+
+additionalProperties: false
+
+required:
+ - compatible
+ - reg
+ - vdd-supply
+
+examples:
+ - |
+ #include <dt-bindings/interrupt-controller/irq.h>
+ i2c {
+ #address-cells = <1>;
+ #size-cells = <0>;
+
+ light-sensor@44 {
+ compatible = "ti,opt4048";
+ reg = <0x44>;
+ interrupt-parent = <&pio>;
+ interrupts = <0 8 IRQ_TYPE_EDGE_FALLING>;
+ vdd-supply = <®_vcc_io>;
+ };
+ };
+...
--
2.47.0
^ permalink raw reply related [flat|nested] 10+ messages in thread
* [PATCH 2/2] iio: light: Add support for the TI OPT4048 color sensor
2024-11-30 17:42 [PATCH 1/2] dt-bindings: iio: Add TI OPT4048 color sensor bindings Paul Kocialkowski
@ 2024-11-30 17:42 ` Paul Kocialkowski
2024-12-01 1:29 ` kernel test robot
2024-12-01 11:55 ` Jonathan Cameron
2024-12-02 8:41 ` [PATCH 1/2] dt-bindings: iio: Add TI OPT4048 color sensor bindings Krzysztof Kozlowski
1 sibling, 2 replies; 10+ messages in thread
From: Paul Kocialkowski @ 2024-11-30 17:42 UTC (permalink / raw)
To: linux-iio, devicetree, linux-kernel
Cc: Jonathan Cameron, Lars-Peter Clausen, Rob Herring,
Krzysztof Kozlowski, Conor Dooley, Paul Kocialkowski
The Texas Instruments OPT4048 is a XYZ tristimulus color sensor,
with an additional wide (visible + IR) channel.
This driver implements support for all channels, with configurable
integration time and auto-gain. Both direct reading and
triggered-buffer modes are supported.
Note that the Y channel is also reported as a separate illuminance
channel, for which a scale is provided (following the datasheet) to
convert it to lux units. Falling and rising thresholds are supported
for this channel.
The device's interrupt can be used to sample all channels at the end
of conversion and is optional.
Signed-off-by: Paul Kocialkowski <paulk@sys-base.io>
---
drivers/iio/light/Kconfig | 10 +
drivers/iio/light/Makefile | 1 +
drivers/iio/light/opt4048.c | 1145 +++++++++++++++++++++++++++++++++++
3 files changed, 1156 insertions(+)
create mode 100644 drivers/iio/light/opt4048.c
diff --git a/drivers/iio/light/Kconfig b/drivers/iio/light/Kconfig
index f2f3e414849a..6d0cc1eecae0 100644
--- a/drivers/iio/light/Kconfig
+++ b/drivers/iio/light/Kconfig
@@ -492,6 +492,16 @@ config OPT4001
If built as a dynamically linked module, it will be called
opt4001.
+config OPT4048
+ tristate "Texas Instruments OPT4048 XYZ tristimulus color sensor driver"
+ depends on I2C && PM
+ select IIO_BUFFER
+ select IIO_TRIGGERED_BUFFER
+ help
+ Support for the Texas Instruments OPT4048 XYZ tristimulus color sensor.
+
+ This driver can be built-in or built as a module, called opt4048.
+
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 321010fc0b93..f2031e6236f9 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_OPT4048) += opt4048.o
obj-$(CONFIG_PA12203001) += pa12203001.o
obj-$(CONFIG_ROHM_BU27008) += rohm-bu27008.o
obj-$(CONFIG_ROHM_BU27034) += rohm-bu27034.o
diff --git a/drivers/iio/light/opt4048.c b/drivers/iio/light/opt4048.c
new file mode 100644
index 000000000000..1ad5e6586aad
--- /dev/null
+++ b/drivers/iio/light/opt4048.c
@@ -0,0 +1,1145 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * Copyright 2024 Paul Kocialkowski <paulk@sys-base.io>
+ */
+
+#include <linux/delay.h>
+#include <linux/i2c.h>
+#include <linux/iio/events.h>
+#include <linux/iio/iio.h>
+#include <linux/iio/trigger_consumer.h>
+#include <linux/iio/triggered_buffer.h>
+#include <linux/log2.h>
+#include <linux/module.h>
+#include <linux/pm_runtime.h>
+#include <linux/regulator/consumer.h>
+
+#define OPT4048_CH0_DATA0 0x0
+#define OPT4048_CH0_DATA1 0x1
+#define OPT4048_CH1_DATA0 0x2
+#define OPT4048_CH1_DATA1 0x3
+#define OPT4048_CH2_DATA0 0x4
+#define OPT4048_CH2_DATA1 0x5
+#define OPT4048_CH3_DATA0 0x6
+#define OPT4048_CH3_DATA1 0x7
+
+#define OPT4048_CH_DATA0_MSB_VALUE(v) ((v) & GENMASK(11, 0))
+#define OPT4048_CH_DATA0_EXPONENT_VALUE(v) (((v) & GENMASK(15, 12)) >> 12)
+
+#define OPT4048_CH_DATA1_CRC_VALUE(v) ((v) & GENMASK(3, 0))
+#define OPT4048_CH_DATA1_COUNTER_VALUE(v) (((v) & GENMASK(7, 4)) >> 4)
+#define OPT4048_CH_DATA1_LSB_VALUE(v) (((v) & GENMASK(15, 8)) >> 8)
+
+#define OPT4048_THRESHOLD_L 0x8
+#define OPT4048_THRESHOLD_H 0x9
+
+#define OPT4048_THRESHOLD_RESULT(v) ((v) & GENMASK(11, 0))
+#define OPT4048_THRESHOLD_RESULT_MAX ((1 << 12) - 1)
+#define OPT4048_THRESHOLD_EXPONENT(v) (((v) << 12) & GENMASK(15, 12))
+#define OPT4048_THRESHOLD_EXPONENT_MAX ((1 << 4) - 1)
+
+#define OPT4048_CFG0 0xa
+#define OPT4048_CFG0_FAULT_COUNT_1 0
+#define OPT4048_CFG0_FAULT_COUNT_2 1
+#define OPT4048_CFG0_FAULT_COUNT_4 2
+#define OPT4048_CFG0_FAULT_COUNT_8 3
+#define OPT4048_CFG0_INT_POL_ACTIVE_LOW 0
+#define OPT4048_CFG0_INT_POL_ACTIVE_HIGH BIT(2)
+#define OPT4048_CFG0_LATCH BIT(3)
+#define OPT4048_CFG0_OP_MODE_POWER_DOWN 0
+#define OPT4048_CFG0_OP_MODE_ONESHOT_AUTORANGE (1 << 4)
+#define OPT4048_CFG0_OP_MODE_ONESHOT (2 << 4)
+#define OPT4048_CFG0_OP_MODE_CONTINUOUS (3 << 4)
+#define OPT4048_CFG0_CONVERSION_TIME(v) (((v) << 6) & GENMASK(9, 6))
+#define OPT4048_CFG0_RANGE(v) (((v) << 10) & GENMASK(13, 10))
+#define OPT4048_CFG0_RANGE_AUTO (12 << 10)
+#define OPT4048_CFG0_QWAKE BIT(15)
+
+#define OPT4048_CFG1 0xb
+#define OPT4048_CFG1_I2C_BURST BIT(0)
+#define OPT4048_CFG1_INT_CFG_ALERT 0
+#define OPT4048_CFG1_INT_CFG_DATA_READY_NEXT (1 << 2)
+#define OPT4048_CFG1_INT_CFG_DATA_READY_ALL (3 << 2)
+#define OPT4048_CFG1_INT_DIR_IN 0
+#define OPT4048_CFG1_INT_DIR_OUT BIT(4)
+#define OPT4048_CFG1_THRESHOLD_CH_SEL(i) (((i) << 5) & GENMASK(6, 5))
+#define OPT4048_CFG1_RESERVED (0x80 << 7)
+
+#define OPT4048_STATUS 0xc
+#define OPT4048_STATUS_FLAG_L BIT(0)
+#define OPT4048_STATUS_FLAG_H BIT(1)
+#define OPT4048_STATUS_CONV_READY_FLAG BIT(2)
+#define OPT4048_STATUS_OVERLOAD_FLAG BIT(3)
+
+#define OPT4048_DID 0x11
+#define OPT4048_DID_L_VALUE(v) (((v) & GENMASK(13, 12)) >> 12)
+#define OPT4048_DID_H_VALUE(v) ((v) & GENMASK(11, 0))
+#define OPT4048_DID_VALUE(l, h) (((h) << 2) | (l))
+
+#define OPT4048_DID_OPT4048 0x2084
+
+#define OPT4048_SCALE_ULUX 2150
+
+struct opt4048_sensor_state {
+ bool active;
+ u8 conversion_time_index;
+ u16 threshold_low[2];
+ bool threshold_low_active;
+ u16 threshold_high[2];
+ bool threshold_high_active;
+ u16 status;
+};
+
+struct opt4048_sensor_scan {
+ u32 channels[5];
+ s64 timestamp __aligned(8);
+};
+
+struct opt4048_sensor {
+ struct device *dev;
+ struct i2c_client *i2c_client;
+ struct iio_dev *iio_dev;
+
+ struct regulator *vdd_supply;
+
+ struct opt4048_sensor_state state;
+ struct opt4048_sensor_scan scan;
+ bool scan_sync;
+ struct mutex lock;
+};
+
+static int opt4048_state_configure_cfg0(struct opt4048_sensor *sensor);
+
+static const int opt4048_conversion_time_available[] = {
+ 0, 600,
+ 0, 1000,
+ 0, 1800,
+ 0, 3400,
+ 0, 6500,
+ 0, 12700,
+ 0, 25000,
+ 0, 50000,
+ 0, 100000,
+ 0, 200000,
+ 0, 400000,
+ 0, 800000,
+};
+
+static int opt4048_conversion_time_index(int value, int value_micro)
+{
+ unsigned int count = ARRAY_SIZE(opt4048_conversion_time_available) / 2;
+ unsigned int index;
+ unsigned int i;
+
+ for (i = 0; i < count; i++) {
+ index = i * 2;
+
+ if (opt4048_conversion_time_available[index] != value)
+ continue;
+
+ index++;
+
+ if (opt4048_conversion_time_available[index] != value_micro)
+ continue;
+
+ return (int)i;
+ }
+
+ return -1;
+}
+
+static u32 opt4048_threshold_value(u16 result, u16 exponent)
+{
+ return result << (exponent + 8);
+}
+
+static void opt4048_threshold_convert(u32 value, u16 threshold[2])
+{
+ unsigned int result;
+ unsigned int exponent;
+
+ /*
+ * Threshold values are stored with 12 bits for the mantissa (result)
+ * and 4 bits for an exponent offset, added to 8.
+ */
+
+ result = value >> 8;
+ exponent = ilog2(result);
+
+ if (exponent > 11) {
+ exponent -= 11;
+ result >>= exponent;
+ } else {
+ exponent = 0;
+ }
+
+ threshold[0] = (u16)result;
+ threshold[1] = (u16)exponent;
+}
+
+static u32 opt4048_data_value(u16 result_msb, u16 result_lsb, u16 exponent)
+{
+ return ((result_msb << 8) | result_lsb) << exponent;
+}
+
+static int opt4048_data_read(struct opt4048_sensor *sensor, u8 address,
+ u32 *channel_value)
+{
+ struct i2c_client *i2c_client = sensor->i2c_client;
+ u16 value, result_msb, result_lsb, exponent;
+ unsigned int index = 0;
+ u8 values[4] = { 0 };
+ int ret;
+
+ /* Read all values in one transaction to ensure coherency. */
+ ret = i2c_smbus_read_i2c_block_data(i2c_client, address, 4, values);
+ if (ret < 0)
+ return ret;
+
+ value = values[index] << 8 | values[index + 1];
+ index += 2;
+
+ result_msb = OPT4048_CH_DATA0_MSB_VALUE(value);
+ exponent = OPT4048_CH_DATA0_EXPONENT_VALUE(value);
+
+ value = values[index] << 8 | values[index + 1];
+
+ result_lsb = (u16)OPT4048_CH_DATA1_LSB_VALUE(value);
+
+ *channel_value = opt4048_data_value(result_msb, result_lsb, exponent);
+
+ return 0;
+}
+
+static int opt4048_data_scan(struct opt4048_sensor *sensor,
+ struct opt4048_sensor_scan *scan)
+{
+ struct i2c_client *i2c_client = sensor->i2c_client;
+ unsigned int index = 0;
+ u8 values[16] = { 0 };
+ unsigned int i;
+ int ret;
+
+ /* Read all values in one transaction to ensure coherency. */
+ ret = i2c_smbus_read_i2c_block_data(i2c_client, OPT4048_CH0_DATA0, 16,
+ values);
+ if (ret < 0)
+ return ret;
+
+ for (i = 0; i < 4; i++) {
+ u16 value, result_msb, result_lsb, exponent;
+
+ value = values[index] << 8 | values[index + 1];
+ index += 2;
+
+ result_msb = (u16)OPT4048_CH_DATA0_MSB_VALUE(value);
+ exponent = (u16)OPT4048_CH_DATA0_EXPONENT_VALUE(value);
+
+ value = values[index] << 8 | values[index + 1];
+ index += 2;
+
+ result_lsb = (u16)OPT4048_CH_DATA1_LSB_VALUE(value);
+
+ scan->channels[i] =
+ opt4048_data_value(result_msb, result_lsb, exponent);
+ }
+
+ /* Report illuminance using Y intensity value. */
+ scan->channels[4] = scan->channels[1];
+
+ return 0;
+}
+
+static int opt4048_identify(struct opt4048_sensor *sensor)
+{
+ struct device *dev = sensor->dev;
+ int ret;
+ u16 id, low, high;
+
+ ret = pm_runtime_resume_and_get(dev);
+ if (ret < 0)
+ return ret;
+
+ ret = i2c_smbus_read_word_swapped(sensor->i2c_client, OPT4048_DID);
+ if (ret < 0)
+ goto complete;
+
+ low = (u16)OPT4048_DID_L_VALUE(ret);
+ high = (u16)OPT4048_DID_H_VALUE(ret);
+
+ id = OPT4048_DID_VALUE(low, high);
+
+ switch (id) {
+ case OPT4048_DID_OPT4048:
+ dev_info(dev, "identified OPT4048 sensor\n");
+ ret = 0;
+ break;
+ default:
+ dev_err(dev, "unknown sensor with id: %#x\n", id);
+ ret = -ENODEV;
+ break;
+ }
+
+complete:
+ pm_runtime_put_sync(dev);
+
+ return ret;
+}
+
+static int opt4048_power(struct opt4048_sensor *sensor, bool on)
+{
+ struct opt4048_sensor_state *state = &sensor->state;
+ int ret;
+
+ mutex_lock(&sensor->lock);
+
+ state->active = on;
+ ret = opt4048_state_configure_cfg0(sensor);
+
+ mutex_unlock(&sensor->lock);
+
+ return ret;
+}
+
+/* State */
+
+static int opt4048_state_configure_cfg0(struct opt4048_sensor *sensor)
+{
+ struct opt4048_sensor_state *state = &sensor->state;
+ u16 value;
+ int ret;
+
+ /*
+ * Use the auto-ranging feature instead of manual gain exponent,
+ * use latched window mode for threshold.
+ */
+ value = OPT4048_CFG0_FAULT_COUNT_1 |
+ OPT4048_CFG0_INT_POL_ACTIVE_LOW |
+ OPT4048_CFG0_LATCH |
+ OPT4048_CFG0_CONVERSION_TIME(state->conversion_time_index) |
+ OPT4048_CFG0_RANGE_AUTO;
+
+ if (state->active)
+ value |= OPT4048_CFG0_OP_MODE_CONTINUOUS;
+ else
+ value |= OPT4048_CFG0_OP_MODE_POWER_DOWN;
+
+ ret = i2c_smbus_write_word_swapped(sensor->i2c_client, OPT4048_CFG0,
+ value);
+ if (ret < 0)
+ return ret;
+
+ return 0;
+}
+
+static int opt4048_state_configure_cfg1(struct opt4048_sensor *sensor)
+{
+ u16 value;
+ int ret;
+
+ /* Assign threshold to the Y channel for illuminance. */
+ value = OPT4048_CFG1_I2C_BURST |
+ OPT4048_CFG1_INT_DIR_OUT |
+ OPT4048_CFG1_THRESHOLD_CH_SEL(1) |
+ OPT4048_CFG1_RESERVED;
+
+ if (sensor->scan_sync)
+ value |= OPT4048_CFG1_INT_CFG_DATA_READY_ALL;
+ else
+ value |= OPT4048_CFG1_INT_CFG_ALERT;
+
+ ret = i2c_smbus_write_word_swapped(sensor->i2c_client, OPT4048_CFG1,
+ value);
+ if (ret < 0)
+ return ret;
+
+ return 0;
+}
+
+static int opt4048_state_configure_threshold(struct opt4048_sensor *sensor)
+{
+ struct opt4048_sensor_state *state = &sensor->state;
+ u16 threshold[2];
+ u16 value;
+ int ret;
+
+ if (state->threshold_low_active) {
+ threshold[0] = state->threshold_low[0];
+ threshold[1] = state->threshold_low[1];
+ } else {
+ threshold[0] = 0;
+ threshold[1] = 0;
+ }
+
+ value = OPT4048_THRESHOLD_RESULT(threshold[0]) |
+ OPT4048_THRESHOLD_EXPONENT(threshold[1]);
+
+ ret = i2c_smbus_write_word_swapped(sensor->i2c_client,
+ OPT4048_THRESHOLD_L, value);
+ if (ret < 0)
+ return ret;
+
+ if (state->threshold_high_active) {
+ threshold[0] = state->threshold_high[0];
+ threshold[1] = state->threshold_high[1];
+ } else {
+ threshold[0] = OPT4048_THRESHOLD_RESULT_MAX;
+ threshold[1] = OPT4048_THRESHOLD_EXPONENT_MAX;
+ }
+
+ value = OPT4048_THRESHOLD_RESULT(threshold[0]) |
+ OPT4048_THRESHOLD_EXPONENT(threshold[1]);
+
+ ret = i2c_smbus_write_word_swapped(sensor->i2c_client,
+ OPT4048_THRESHOLD_H, value);
+ if (ret < 0)
+ return ret;
+
+ return 0;
+}
+
+static int opt4048_state_configure(struct opt4048_sensor *sensor)
+{
+ int ret;
+
+ mutex_lock(&sensor->lock);
+
+ ret = opt4048_state_configure_cfg0(sensor);
+ if (ret)
+ goto complete;
+
+ ret = opt4048_state_configure_cfg1(sensor);
+ if (ret)
+ goto complete;
+
+ ret = opt4048_state_configure_threshold(sensor);
+ if (ret)
+ goto complete;
+
+complete:
+ mutex_unlock(&sensor->lock);
+
+ return ret;
+}
+
+static void opt4048_state_reset(struct opt4048_sensor *sensor)
+{
+ struct opt4048_sensor_state *state = &sensor->state;
+
+ state->active = false;
+
+ /* Start with a 25 ms integration time. */
+ state->conversion_time_index = 6;
+
+ state->threshold_low_active = false;
+ state->threshold_high_active = false;
+}
+
+/* IIO */
+
+static const struct iio_event_spec opt4048_iio_events[] = {
+ {
+ .type = IIO_EV_TYPE_THRESH,
+ .dir = IIO_EV_DIR_RISING,
+ .mask_separate =
+ BIT(IIO_EV_INFO_ENABLE) |
+ BIT(IIO_EV_INFO_VALUE),
+ },
+ {
+ .type = IIO_EV_TYPE_THRESH,
+ .dir = IIO_EV_DIR_FALLING,
+ .mask_separate =
+ BIT(IIO_EV_INFO_ENABLE) |
+ BIT(IIO_EV_INFO_VALUE),
+ },
+};
+
+static const struct iio_chan_spec opt4048_iio_channels[] = {
+ {
+ .type = IIO_INTENSITY,
+ .channel2 = IIO_MOD_X,
+ .address = OPT4048_CH0_DATA0,
+ .modified = 1,
+
+ .scan_index = 0,
+ .scan_type = {
+ .sign = 'u',
+ .realbits = 28,
+ .storagebits = 32,
+ .endianness = IIO_LE,
+ },
+
+ .info_mask_separate =
+ BIT(IIO_CHAN_INFO_RAW),
+ .info_mask_shared_by_all =
+ BIT(IIO_CHAN_INFO_INT_TIME),
+ .info_mask_shared_by_all_available =
+ BIT(IIO_CHAN_INFO_INT_TIME),
+ },
+ {
+ .type = IIO_INTENSITY,
+ .channel2 = IIO_MOD_Y,
+ .address = OPT4048_CH1_DATA0,
+ .modified = 1,
+
+ .scan_index = 1,
+ .scan_type = {
+ .sign = 'u',
+ .realbits = 28,
+ .storagebits = 32,
+ .endianness = IIO_LE,
+ },
+
+ .info_mask_separate =
+ BIT(IIO_CHAN_INFO_RAW),
+ .info_mask_shared_by_all =
+ BIT(IIO_CHAN_INFO_INT_TIME),
+ .info_mask_shared_by_all_available =
+ BIT(IIO_CHAN_INFO_INT_TIME),
+ },
+ {
+ .type = IIO_INTENSITY,
+ .channel2 = IIO_MOD_Z,
+ .address = OPT4048_CH2_DATA0,
+ .modified = 1,
+
+ .scan_index = 2,
+ .scan_type = {
+ .sign = 'u',
+ .realbits = 28,
+ .storagebits = 32,
+ .endianness = IIO_LE,
+ },
+
+ .info_mask_separate =
+ BIT(IIO_CHAN_INFO_RAW),
+ .info_mask_shared_by_all =
+ BIT(IIO_CHAN_INFO_INT_TIME),
+ .info_mask_shared_by_all_available =
+ BIT(IIO_CHAN_INFO_INT_TIME),
+ },
+ {
+ .type = IIO_INTENSITY,
+ .channel2 = IIO_MOD_LIGHT_BOTH,
+ .address = OPT4048_CH3_DATA0,
+ .modified = 1,
+
+ .scan_index = 3,
+ .scan_type = {
+ .sign = 'u',
+ .realbits = 28,
+ .storagebits = 32,
+ .endianness = IIO_LE,
+ },
+
+ .info_mask_separate =
+ BIT(IIO_CHAN_INFO_RAW),
+ .info_mask_shared_by_all =
+ BIT(IIO_CHAN_INFO_INT_TIME),
+ .info_mask_shared_by_all_available =
+ BIT(IIO_CHAN_INFO_INT_TIME),
+ },
+ {
+ .type = IIO_LIGHT,
+ .address = OPT4048_CH1_DATA0,
+
+ .scan_index = 4,
+ .scan_type = {
+ .sign = 'u',
+ .realbits = 28,
+ .storagebits = 32,
+ .endianness = IIO_LE,
+ },
+
+ .info_mask_separate =
+ BIT(IIO_CHAN_INFO_RAW) |
+ BIT(IIO_CHAN_INFO_SCALE),
+ .info_mask_shared_by_all =
+ BIT(IIO_CHAN_INFO_INT_TIME),
+ .info_mask_shared_by_all_available =
+ BIT(IIO_CHAN_INFO_INT_TIME),
+ },
+ IIO_CHAN_SOFT_TIMESTAMP(5),
+};
+
+static int opt4048_iio_read_raw(struct iio_dev *iio_dev,
+ struct iio_chan_spec const *channel,
+ int *value_first, int *value_second, long mask)
+{
+ struct opt4048_sensor *sensor = iio_priv(iio_dev);
+ struct opt4048_sensor_state *state = &sensor->state;
+ struct device *dev = sensor->dev;
+ int ret;
+
+ switch (mask) {
+ case IIO_CHAN_INFO_RAW:
+ unsigned int scan_index;
+
+ ret = iio_device_claim_direct_mode(iio_dev);
+ if (ret)
+ return ret;
+
+ ret = pm_runtime_resume_and_get(dev);
+ if (ret < 0)
+ goto release_direct_mode;
+
+ if (sensor->scan_sync) {
+ scan_index = channel->scan_index;
+
+ mutex_lock(&sensor->lock);
+ *value_first = (int)sensor->scan.channels[scan_index];
+ mutex_unlock(&sensor->lock);
+ } else {
+ ret = opt4048_data_read(sensor, channel->address,
+ (u32 *)value_first);
+ }
+
+ pm_runtime_mark_last_busy(dev);
+ pm_runtime_put_autosuspend(dev);
+
+release_direct_mode:
+ iio_device_release_direct_mode(iio_dev);
+
+ if (ret < 0)
+ return ret;
+
+ return IIO_VAL_INT;
+
+ case IIO_CHAN_INFO_SCALE:
+ *value_first = 0;
+ *value_second = OPT4048_SCALE_ULUX;
+
+ return IIO_VAL_INT_PLUS_MICRO;
+
+ case IIO_CHAN_INFO_INT_TIME:
+ unsigned int index = 2 * state->conversion_time_index;
+
+ *value_first = opt4048_conversion_time_available[index];
+ index++;
+ *value_second = opt4048_conversion_time_available[index];
+
+ return IIO_VAL_INT_PLUS_MICRO;
+
+ default:
+ return -EINVAL;
+ }
+}
+
+static int opt4048_iio_read_avail(struct iio_dev *iio_dev,
+ struct iio_chan_spec const *channel,
+ const int **values, int *type, int *length,
+ long mask)
+{
+ switch (mask) {
+ case IIO_CHAN_INFO_INT_TIME:
+ *length = ARRAY_SIZE(opt4048_conversion_time_available);
+ *values = opt4048_conversion_time_available;
+ *type = IIO_VAL_INT_PLUS_MICRO;
+ return IIO_AVAIL_LIST;
+
+ default:
+ return -EINVAL;
+ }
+}
+
+static int opt4048_iio_write_raw(struct iio_dev *iio_dev,
+ struct iio_chan_spec const *channel,
+ int value_first, int value_second, long mask)
+{
+ struct opt4048_sensor *sensor = iio_priv(iio_dev);
+ struct opt4048_sensor_state *state = &sensor->state;
+ int ret;
+
+ switch (mask) {
+ case IIO_CHAN_INFO_INT_TIME:
+ ret = opt4048_conversion_time_index(value_first, value_second);
+ if (ret < 0)
+ return -EINVAL;
+
+ state->conversion_time_index = (u16)ret;
+
+ if (pm_runtime_suspended(sensor->dev))
+ return 0;
+
+ ret = opt4048_state_configure_cfg0(sensor);
+ if (ret)
+ return ret;
+
+ return 0;
+
+ default:
+ return -EINVAL;
+ }
+}
+
+static int opt4048_iio_read_event_value(struct iio_dev *iio_dev,
+ struct iio_chan_spec const *channel,
+ enum iio_event_type type,
+ enum iio_event_direction direction,
+ enum iio_event_info info,
+ int *value_first, int *value_second)
+{
+ struct opt4048_sensor *sensor = iio_priv(iio_dev);
+ struct opt4048_sensor_state *state = &sensor->state;
+ u32 value;
+
+ switch (direction) {
+ case IIO_EV_DIR_RISING:
+ value = opt4048_threshold_value(state->threshold_high[0],
+ state->threshold_high[1]);
+ *value_first = (int)value;
+
+ return IIO_VAL_INT;
+
+ case IIO_EV_DIR_FALLING:
+ value = opt4048_threshold_value(state->threshold_low[0],
+ state->threshold_low[1]);
+ *value_first = (int)value;
+
+ return IIO_VAL_INT;
+
+ default:
+ return -EINVAL;
+ }
+}
+
+static int opt4048_iio_write_event_value(struct iio_dev *iio_dev,
+ struct iio_chan_spec const *channel,
+ enum iio_event_type type,
+ enum iio_event_direction direction,
+ enum iio_event_info info,
+ int value_first, int value_second)
+{
+ struct opt4048_sensor *sensor = iio_priv(iio_dev);
+ struct opt4048_sensor_state *state = &sensor->state;
+ u32 value;
+ int ret;
+
+ switch (direction) {
+ case IIO_EV_DIR_RISING:
+ value = (u32)value_first;
+ opt4048_threshold_convert(value, state->threshold_high);
+ break;
+
+ case IIO_EV_DIR_FALLING:
+ value = (u32)value_first;
+ opt4048_threshold_convert(value, state->threshold_low);
+ break;
+
+ default:
+ return -EINVAL;
+ }
+
+ if (pm_runtime_suspended(sensor->dev))
+ return 0;
+
+ ret = opt4048_state_configure_threshold(sensor);
+ if (ret)
+ return ret;
+
+ return 0;
+}
+
+static int opt4048_iio_read_event_config(struct iio_dev *iio_dev,
+ struct iio_chan_spec const *channel,
+ enum iio_event_type type,
+ enum iio_event_direction direction)
+{
+ struct opt4048_sensor *sensor = iio_priv(iio_dev);
+ struct opt4048_sensor_state *state = &sensor->state;
+
+ switch (direction) {
+ case IIO_EV_DIR_RISING:
+ return state->threshold_high_active;
+
+ case IIO_EV_DIR_FALLING:
+ return state->threshold_low_active;
+
+ default:
+ return -EINVAL;
+ }
+}
+
+static int opt4048_iio_write_event_config(struct iio_dev *iio_dev,
+ struct iio_chan_spec const *channel,
+ enum iio_event_type type,
+ enum iio_event_direction direction,
+ int active)
+{
+ struct opt4048_sensor *sensor = iio_priv(iio_dev);
+ struct opt4048_sensor_state *state = &sensor->state;
+ int ret = 0;
+
+ /* Threshold active are read in IRQ thread. */
+ mutex_lock(&sensor->lock);
+
+ switch (direction) {
+ case IIO_EV_DIR_RISING:
+ state->threshold_high_active = !!active;
+ break;
+
+ case IIO_EV_DIR_FALLING:
+ state->threshold_low_active = !!active;
+ break;
+
+ default:
+ ret = -EINVAL;
+ goto complete;
+ }
+
+ if (pm_runtime_suspended(sensor->dev))
+ goto complete;
+
+ ret = opt4048_state_configure_threshold(sensor);
+
+complete:
+ mutex_unlock(&sensor->lock);
+
+ return ret;
+}
+
+static const struct iio_info opt4048_iio_info = {
+ .read_raw = opt4048_iio_read_raw,
+ .read_avail = opt4048_iio_read_avail,
+ .write_raw = opt4048_iio_write_raw,
+ .read_event_value = opt4048_iio_read_event_value,
+ .write_event_value = opt4048_iio_write_event_value,
+ .read_event_config = opt4048_iio_read_event_config,
+ .write_event_config = opt4048_iio_write_event_config,
+};
+
+static irqreturn_t opt4048_iio_buffer_trigger(int irq, void *data)
+{
+ struct iio_poll_func *poll_func = data;
+ struct iio_dev *iio_dev = poll_func->indio_dev;
+ struct opt4048_sensor *sensor = iio_priv(iio_dev);
+ struct opt4048_sensor_scan scan = { 0 };
+ s64 timestamp;
+ unsigned int index = 0;
+ unsigned int i;
+ int ret;
+
+ /* Capture timestamp just before reading values. */
+ timestamp = iio_get_time_ns(iio_dev);
+
+ mutex_lock(&sensor->lock);
+
+ if (!sensor->scan_sync) {
+ ret = opt4048_data_scan(sensor, &sensor->scan);
+ if (ret)
+ goto complete;
+ }
+
+ for_each_set_bit(i, iio_dev->active_scan_mask, iio_dev->masklength) {
+ /* Assume scan index matches array index. */
+ const struct iio_chan_spec *channel = &opt4048_iio_channels[i];
+ unsigned int scan_index = channel->scan_index;
+
+ /* Only active channels are reported, in order. */
+ scan.channels[index] = sensor->scan.channels[scan_index];
+ index++;
+ }
+
+ iio_push_to_buffers_with_timestamp(iio_dev, &scan, timestamp);
+
+complete:
+ mutex_unlock(&sensor->lock);
+
+ iio_trigger_notify_done(iio_dev->trig);
+
+ return IRQ_HANDLED;
+}
+
+static int opt4048_iio_buffer_preenable(struct iio_dev *iio_dev)
+{
+ struct opt4048_sensor *sensor = iio_priv(iio_dev);
+ struct device *dev = sensor->dev;
+
+ return pm_runtime_resume_and_get(dev);
+}
+
+static int opt4048_iio_buffer_postdisable(struct iio_dev *iio_dev)
+{
+ struct opt4048_sensor *sensor = iio_priv(iio_dev);
+ struct device *dev = sensor->dev;
+
+ pm_runtime_mark_last_busy(dev);
+ pm_runtime_put_autosuspend(dev);
+
+ return 0;
+}
+static const struct iio_buffer_setup_ops opt4048_iio_buffer_setup_ops = {
+ .preenable = opt4048_iio_buffer_preenable,
+ .postdisable = opt4048_iio_buffer_postdisable,
+};
+
+static int opt4048_iio_setup(struct opt4048_sensor *sensor)
+{
+ struct iio_chan_spec *channels;
+ struct iio_dev *iio_dev = sensor->iio_dev;
+ struct device *dev = sensor->dev;
+ unsigned int channels_count;
+ int ret;
+
+ channels_count = ARRAY_SIZE(opt4048_iio_channels);
+
+ if (sensor->i2c_client->irq > 0) {
+ channels = devm_kzalloc(dev, sizeof(opt4048_iio_channels),
+ GFP_KERNEL);
+
+ memcpy(channels, opt4048_iio_channels,
+ sizeof(opt4048_iio_channels));
+
+ /* Attach threshold events to the illuminance channel. */
+ channels[3].event_spec = opt4048_iio_events;
+ channels[3].num_event_specs = ARRAY_SIZE(opt4048_iio_events);
+ } else {
+ /* Threshold events are not available without an irq. */
+ channels = (struct iio_chan_spec *)opt4048_iio_channels;
+ }
+
+ iio_dev->info = &opt4048_iio_info;
+ iio_dev->name = "opt4048";
+ iio_dev->channels = channels;
+ iio_dev->num_channels = channels_count;
+ iio_dev->modes = INDIO_DIRECT_MODE;
+
+ ret = devm_iio_triggered_buffer_setup(dev, iio_dev, NULL,
+ opt4048_iio_buffer_trigger,
+ &opt4048_iio_buffer_setup_ops);
+ if (ret) {
+ dev_err(dev, "failed to setup iio triggered buffer\n");
+ return ret;
+ }
+
+ ret = iio_device_register(iio_dev);
+ if (ret) {
+ dev_err(dev, "failed to register iio device\n");
+ return ret;
+ }
+
+ return 0;
+}
+
+static void opt4048_iio_cleanup(struct opt4048_sensor *sensor)
+{
+ iio_device_unregister(sensor->iio_dev);
+}
+
+/* IRQ */
+
+static irqreturn_t opt4048_irq(int irq, void *data)
+{
+ struct opt4048_sensor *sensor = data;
+ struct opt4048_sensor_state *state = &sensor->state;
+ struct iio_dev *iio_dev = sensor->iio_dev;
+ bool threshold_rising;
+ bool threshold_falling;
+ s64 timestamp;
+ u64 code;
+ u16 status;
+ int ret;
+
+ timestamp = iio_get_time_ns(iio_dev);
+
+ ret = i2c_smbus_read_word_swapped(sensor->i2c_client, OPT4048_STATUS);
+ if (ret < 0)
+ return IRQ_HANDLED;
+
+ status = (u16)ret;
+
+ mutex_lock(&sensor->lock);
+
+ if (!state->active)
+ goto complete;
+
+ if (status & OPT4048_STATUS_CONV_READY_FLAG)
+ opt4048_data_scan(sensor, &sensor->scan);
+
+ threshold_falling = (status & OPT4048_STATUS_FLAG_L) &&
+ !(state->status & OPT4048_STATUS_FLAG_L);
+
+ if (state->threshold_low_active && threshold_falling) {
+ code = IIO_UNMOD_EVENT_CODE(IIO_LIGHT, 3, IIO_EV_TYPE_THRESH,
+ IIO_EV_DIR_FALLING);
+ iio_push_event(iio_dev, code, timestamp);
+ }
+
+ threshold_rising = (status & OPT4048_STATUS_FLAG_H) &&
+ !(state->status & OPT4048_STATUS_FLAG_H);
+
+ if (state->threshold_high_active && threshold_rising) {
+ code = IIO_UNMOD_EVENT_CODE(IIO_LIGHT, 3, IIO_EV_TYPE_THRESH,
+ IIO_EV_DIR_RISING);
+ iio_push_event(iio_dev, code, timestamp);
+ }
+
+ state->status = status;
+
+complete:
+ mutex_unlock(&sensor->lock);
+
+ return IRQ_HANDLED;
+}
+
+/* PM */
+
+static int opt4048_suspend(struct device *dev)
+{
+ struct opt4048_sensor *sensor = dev_get_drvdata(dev);
+ int ret;
+
+ ret = opt4048_power(sensor, 0);
+ if (ret)
+ goto error;
+
+ ret = regulator_disable(sensor->vdd_supply);
+ if (ret)
+ goto error;
+
+ return 0;
+
+error:
+ return -EAGAIN;
+}
+
+static int opt4048_resume(struct device *dev)
+{
+ struct opt4048_sensor *sensor = dev_get_drvdata(dev);
+ unsigned long sleep_min;
+ unsigned int index;
+ int ret;
+
+ ret = regulator_enable(sensor->vdd_supply);
+ if (ret)
+ goto error;
+
+ /* Wait for the regulator to settle and the chip to power-on. */
+ udelay(30);
+
+ ret = opt4048_state_configure(sensor);
+ if (ret)
+ goto error_regulator;
+
+ ret = opt4048_power(sensor, 1);
+ if (ret)
+ goto error_regulator;
+
+ /* Wait for conversion to be ready for all channels. */
+ index = 2 * sensor->state.conversion_time_index + 1;
+ sleep_min = opt4048_conversion_time_available[index] * 4;
+
+ usleep_range(sleep_min, 5 * sleep_min / 4);
+
+ return 0;
+
+error_regulator:
+ regulator_disable(sensor->vdd_supply);
+
+error:
+ return -EAGAIN;
+}
+
+static const struct dev_pm_ops opt4048_pm_ops = {
+ .runtime_suspend = opt4048_suspend,
+ .runtime_resume = opt4048_resume,
+};
+
+/* I2C */
+
+static int opt4048_probe(struct i2c_client *client)
+{
+ struct device *dev = &client->dev;
+ struct opt4048_sensor *sensor;
+ struct iio_dev *iio_dev;
+ int irq = client->irq;
+ int ret;
+
+ iio_dev = devm_iio_device_alloc(dev, sizeof(*sensor));
+ if (!iio_dev)
+ return -ENOMEM;
+
+ sensor = iio_priv(iio_dev);
+
+ sensor->dev = dev;
+ sensor->i2c_client = client;
+ sensor->iio_dev = iio_dev;
+
+ i2c_set_clientdata(client, sensor);
+
+ mutex_init(&sensor->lock);
+
+ sensor->vdd_supply = devm_regulator_get(dev, "vdd");
+ if (IS_ERR(sensor->vdd_supply)) {
+ dev_err(dev, "failed to get VDD regulator\n");
+ return PTR_ERR(sensor->vdd_supply);
+ }
+
+ opt4048_state_reset(sensor);
+
+ devm_pm_runtime_enable(dev);
+
+ ret = opt4048_identify(sensor);
+ if (ret)
+ return ret;
+
+ pm_runtime_set_autosuspend_delay(dev, 5000);
+ pm_runtime_use_autosuspend(dev);
+
+ if (irq > 0) {
+ ret = devm_request_threaded_irq(dev, irq, NULL, opt4048_irq,
+ IRQF_TRIGGER_FALLING |
+ IRQF_ONESHOT,
+ "opt4048", sensor);
+ if (ret) {
+ dev_err(dev, "failed to request irq\n");
+ return ret;
+ }
+
+ sensor->scan_sync = true;
+ }
+
+ ret = opt4048_iio_setup(sensor);
+ if (ret)
+ return ret;
+
+ return 0;
+}
+
+static void opt4048_remove(struct i2c_client *client)
+{
+ struct opt4048_sensor *sensor = i2c_get_clientdata(client);
+
+ opt4048_iio_cleanup(sensor);
+}
+
+static const struct i2c_device_id opt4048_id[] = {
+ { "opt4048" },
+ { }
+};
+
+MODULE_DEVICE_TABLE(i2c, opt4048_id);
+
+static const struct of_device_id opt4048_of_match[] = {
+ { .compatible = "ti,opt4048" },
+ { }
+};
+
+MODULE_DEVICE_TABLE(of, opt4048_of_match);
+
+static struct i2c_driver opt4048_driver = {
+ .driver = {
+ .name = "opt4048",
+ .of_match_table = opt4048_of_match,
+ .pm = &opt4048_pm_ops,
+ },
+ .id_table = opt4048_id,
+ .probe = opt4048_probe,
+ .remove = opt4048_remove,
+};
+
+module_i2c_driver(opt4048_driver);
+
+MODULE_AUTHOR("Paul Kocialkowski <paulk@sys-base.io>");
+MODULE_DESCRIPTION("Texas Instruments OPT4048 XYZ tristimulus color sensor driver");
+MODULE_LICENSE("GPL");
--
2.47.0
^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [PATCH 2/2] iio: light: Add support for the TI OPT4048 color sensor
2024-11-30 17:42 ` [PATCH 2/2] iio: light: Add support for the TI OPT4048 color sensor Paul Kocialkowski
@ 2024-12-01 1:29 ` kernel test robot
2024-12-01 11:55 ` Jonathan Cameron
1 sibling, 0 replies; 10+ messages in thread
From: kernel test robot @ 2024-12-01 1:29 UTC (permalink / raw)
To: Paul Kocialkowski, linux-iio, devicetree, linux-kernel
Cc: llvm, oe-kbuild-all, Jonathan Cameron, Lars-Peter Clausen,
Rob Herring, Krzysztof Kozlowski, Conor Dooley, Paul Kocialkowski
Hi Paul,
kernel test robot noticed the following build errors:
[auto build test ERROR on jic23-iio/togreg]
[also build test ERROR on linus/master v6.12 next-20241128]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]
url: https://github.com/intel-lab-lkp/linux/commits/Paul-Kocialkowski/iio-light-Add-support-for-the-TI-OPT4048-color-sensor/20241201-014458
base: https://git.kernel.org/pub/scm/linux/kernel/git/jic23/iio.git togreg
patch link: https://lore.kernel.org/r/20241130174212.3298371-2-paulk%40sys-base.io
patch subject: [PATCH 2/2] iio: light: Add support for the TI OPT4048 color sensor
config: um-allmodconfig (https://download.01.org/0day-ci/archive/20241201/202412010950.qWyHjojX-lkp@intel.com/config)
compiler: clang version 20.0.0git (https://github.com/llvm/llvm-project 592c0fe55f6d9a811028b5f3507be91458ab2713)
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20241201/202412010950.qWyHjojX-lkp@intel.com/reproduce)
If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202412010950.qWyHjojX-lkp@intel.com/
All error/warnings (new ones prefixed by >>):
In file included from drivers/iio/light/opt4048.c:7:
In file included from include/linux/i2c.h:19:
In file included from include/linux/regulator/consumer.h:35:
In file included from include/linux/suspend.h:5:
In file included from include/linux/swap.h:9:
In file included from include/linux/memcontrol.h:13:
In file included from include/linux/cgroup.h:26:
In file included from include/linux/kernel_stat.h:8:
In file included from include/linux/interrupt.h:11:
In file included from include/linux/hardirq.h:11:
In file included from arch/um/include/asm/hardirq.h:5:
In file included from include/asm-generic/hardirq.h:17:
In file included from include/linux/irq.h:20:
In file included from include/linux/io.h:14:
In file included from arch/um/include/asm/io.h:24:
include/asm-generic/io.h:548:31: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
548 | val = __raw_readb(PCI_IOBASE + addr);
| ~~~~~~~~~~ ^
include/asm-generic/io.h:561:61: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
561 | val = __le16_to_cpu((__le16 __force)__raw_readw(PCI_IOBASE + addr));
| ~~~~~~~~~~ ^
include/uapi/linux/byteorder/little_endian.h:37:51: note: expanded from macro '__le16_to_cpu'
37 | #define __le16_to_cpu(x) ((__force __u16)(__le16)(x))
| ^
In file included from drivers/iio/light/opt4048.c:7:
In file included from include/linux/i2c.h:19:
In file included from include/linux/regulator/consumer.h:35:
In file included from include/linux/suspend.h:5:
In file included from include/linux/swap.h:9:
In file included from include/linux/memcontrol.h:13:
In file included from include/linux/cgroup.h:26:
In file included from include/linux/kernel_stat.h:8:
In file included from include/linux/interrupt.h:11:
In file included from include/linux/hardirq.h:11:
In file included from arch/um/include/asm/hardirq.h:5:
In file included from include/asm-generic/hardirq.h:17:
In file included from include/linux/irq.h:20:
In file included from include/linux/io.h:14:
In file included from arch/um/include/asm/io.h:24:
include/asm-generic/io.h:574:61: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
574 | val = __le32_to_cpu((__le32 __force)__raw_readl(PCI_IOBASE + addr));
| ~~~~~~~~~~ ^
include/uapi/linux/byteorder/little_endian.h:35:51: note: expanded from macro '__le32_to_cpu'
35 | #define __le32_to_cpu(x) ((__force __u32)(__le32)(x))
| ^
In file included from drivers/iio/light/opt4048.c:7:
In file included from include/linux/i2c.h:19:
In file included from include/linux/regulator/consumer.h:35:
In file included from include/linux/suspend.h:5:
In file included from include/linux/swap.h:9:
In file included from include/linux/memcontrol.h:13:
In file included from include/linux/cgroup.h:26:
In file included from include/linux/kernel_stat.h:8:
In file included from include/linux/interrupt.h:11:
In file included from include/linux/hardirq.h:11:
In file included from arch/um/include/asm/hardirq.h:5:
In file included from include/asm-generic/hardirq.h:17:
In file included from include/linux/irq.h:20:
In file included from include/linux/io.h:14:
In file included from arch/um/include/asm/io.h:24:
include/asm-generic/io.h:585:33: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
585 | __raw_writeb(value, PCI_IOBASE + addr);
| ~~~~~~~~~~ ^
include/asm-generic/io.h:595:59: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
595 | __raw_writew((u16 __force)cpu_to_le16(value), PCI_IOBASE + addr);
| ~~~~~~~~~~ ^
include/asm-generic/io.h:605:59: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
605 | __raw_writel((u32 __force)cpu_to_le32(value), PCI_IOBASE + addr);
| ~~~~~~~~~~ ^
include/asm-generic/io.h:693:20: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
693 | readsb(PCI_IOBASE + addr, buffer, count);
| ~~~~~~~~~~ ^
include/asm-generic/io.h:701:20: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
701 | readsw(PCI_IOBASE + addr, buffer, count);
| ~~~~~~~~~~ ^
include/asm-generic/io.h:709:20: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
709 | readsl(PCI_IOBASE + addr, buffer, count);
| ~~~~~~~~~~ ^
include/asm-generic/io.h:718:21: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
718 | writesb(PCI_IOBASE + addr, buffer, count);
| ~~~~~~~~~~ ^
include/asm-generic/io.h:727:21: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
727 | writesw(PCI_IOBASE + addr, buffer, count);
| ~~~~~~~~~~ ^
include/asm-generic/io.h:736:21: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
736 | writesl(PCI_IOBASE + addr, buffer, count);
| ~~~~~~~~~~ ^
In file included from drivers/iio/light/opt4048.c:7:
In file included from include/linux/i2c.h:19:
In file included from include/linux/regulator/consumer.h:35:
In file included from include/linux/suspend.h:5:
In file included from include/linux/swap.h:9:
In file included from include/linux/memcontrol.h:21:
In file included from include/linux/mm.h:2213:
include/linux/vmstat.h:518:36: warning: arithmetic between different enumeration types ('enum node_stat_item' and 'enum lru_list') [-Wenum-enum-conversion]
518 | return node_stat_name(NR_LRU_BASE + lru) + 3; // skip "nr_"
| ~~~~~~~~~~~ ^ ~~~
>> drivers/iio/light/opt4048.c:576:3: warning: label followed by a declaration is a C23 extension [-Wc23-extensions]
576 | unsigned int scan_index;
| ^
drivers/iio/light/opt4048.c:615:3: warning: label followed by a declaration is a C23 extension [-Wc23-extensions]
615 | unsigned int index = 2 * state->conversion_time_index;
| ^
>> drivers/iio/light/opt4048.c:808:24: error: incompatible function pointer types initializing 'int (*)(struct iio_dev *, const struct iio_chan_spec *, enum iio_event_type, enum iio_event_direction, bool)' (aka 'int (*)(struct iio_dev *, const struct iio_chan_spec *, enum iio_event_type, enum iio_event_direction, _Bool)') with an expression of type 'int (struct iio_dev *, const struct iio_chan_spec *, enum iio_event_type, enum iio_event_direction, int)' [-Wincompatible-function-pointer-types]
808 | .write_event_config = opt4048_iio_write_event_config,
| ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
15 warnings and 1 error generated.
vim +808 drivers/iio/light/opt4048.c
800
801 static const struct iio_info opt4048_iio_info = {
802 .read_raw = opt4048_iio_read_raw,
803 .read_avail = opt4048_iio_read_avail,
804 .write_raw = opt4048_iio_write_raw,
805 .read_event_value = opt4048_iio_read_event_value,
806 .write_event_value = opt4048_iio_write_event_value,
807 .read_event_config = opt4048_iio_read_event_config,
> 808 .write_event_config = opt4048_iio_write_event_config,
809 };
810
--
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 2/2] iio: light: Add support for the TI OPT4048 color sensor
2024-11-30 17:42 ` [PATCH 2/2] iio: light: Add support for the TI OPT4048 color sensor Paul Kocialkowski
2024-12-01 1:29 ` kernel test robot
@ 2024-12-01 11:55 ` Jonathan Cameron
2024-12-01 17:56 ` Paul Kocialkowski
1 sibling, 1 reply; 10+ messages in thread
From: Jonathan Cameron @ 2024-12-01 11:55 UTC (permalink / raw)
To: Paul Kocialkowski
Cc: linux-iio, devicetree, linux-kernel, Lars-Peter Clausen,
Rob Herring, Krzysztof Kozlowski, Conor Dooley
On Sat, 30 Nov 2024 18:42:12 +0100
Paul Kocialkowski <paulk@sys-base.io> wrote:
> The Texas Instruments OPT4048 is a XYZ tristimulus color sensor,
> with an additional wide (visible + IR) channel.
>
> This driver implements support for all channels, with configurable
> integration time and auto-gain. Both direct reading and
> triggered-buffer modes are supported.
>
> Note that the Y channel is also reported as a separate illuminance
> channel, for which a scale is provided (following the datasheet) to
> convert it to lux units. Falling and rising thresholds are supported
> for this channel.
>
> The device's interrupt can be used to sample all channels at the end
> of conversion and is optional.
>
> Signed-off-by: Paul Kocialkowski <paulk@sys-base.io>
Hi Paul,
Various comments inline. Most significant is that this seems to be
suitable for a simple dataready trigger that will make your various
interrupt and non interrupt flows more similar.
Jonathan
> diff --git a/drivers/iio/light/Makefile b/drivers/iio/light/Makefile
> index 321010fc0b93..f2031e6236f9 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_OPT4048) += opt4048.o
> obj-$(CONFIG_PA12203001) += pa12203001.o
> obj-$(CONFIG_ROHM_BU27008) += rohm-bu27008.o
> obj-$(CONFIG_ROHM_BU27034) += rohm-bu27034.o
> diff --git a/drivers/iio/light/opt4048.c b/drivers/iio/light/opt4048.c
> new file mode 100644
> index 000000000000..1ad5e6586aad
> --- /dev/null
> +++ b/drivers/iio/light/opt4048.c
> @@ -0,0 +1,1145 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/*
> + * Copyright 2024 Paul Kocialkowski <paulk@sys-base.io>
> + */
> +
> +#include <linux/delay.h>
> +#include <linux/i2c.h>
> +#include <linux/iio/events.h>
> +#include <linux/iio/iio.h>
> +#include <linux/iio/trigger_consumer.h>
> +#include <linux/iio/triggered_buffer.h>
> +#include <linux/log2.h>
> +#include <linux/module.h>
> +#include <linux/pm_runtime.h>
> +#include <linux/regulator/consumer.h>
> +
> +#define OPT4048_CH0_DATA0 0x0
> +#define OPT4048_CH0_DATA1 0x1
> +#define OPT4048_CH1_DATA0 0x2
> +#define OPT4048_CH1_DATA1 0x3
> +#define OPT4048_CH2_DATA0 0x4
> +#define OPT4048_CH2_DATA1 0x5
> +#define OPT4048_CH3_DATA0 0x6
> +#define OPT4048_CH3_DATA1 0x7
> +
> +#define OPT4048_CH_DATA0_MSB_VALUE(v) ((v) & GENMASK(11, 0))
> +#define OPT4048_CH_DATA0_EXPONENT_VALUE(v) (((v) & GENMASK(15, 12)) >> 12)
For all these - just define the masks and use FIELD_GET() / FIELD_PREP()
inline. Masks should be defined with names that make it clear what they are
masking and in what registers.
> +
> +#define OPT4048_CH_DATA1_CRC_VALUE(v) ((v) & GENMASK(3, 0))
> +#define OPT4048_CH_DATA1_COUNTER_VALUE(v) (((v) & GENMASK(7, 4)) >> 4)
> +#define OPT4048_CH_DATA1_LSB_VALUE(v) (((v) & GENMASK(15, 8)) >> 8)
> +
> +#define OPT4048_THRESHOLD_L 0x8
> +#define OPT4048_THRESHOLD_H 0x9
> +
> +#define OPT4048_THRESHOLD_RESULT(v) ((v) & GENMASK(11, 0))
> +#define OPT4048_THRESHOLD_RESULT_MAX ((1 << 12) - 1)
> +#define OPT4048_THRESHOLD_EXPONENT(v) (((v) << 12) & GENMASK(15, 12))
> +#define OPT4048_THRESHOLD_EXPONENT_MAX ((1 << 4) - 1)
> +
> +#define OPT4048_CFG0 0xa
> +#define OPT4048_CFG0_FAULT_COUNT_1 0
> +#define OPT4048_CFG0_FAULT_COUNT_2 1
> +#define OPT4048_CFG0_FAULT_COUNT_4 2
> +#define OPT4048_CFG0_FAULT_COUNT_8 3
> +#define OPT4048_CFG0_INT_POL_ACTIVE_LOW 0
> +#define OPT4048_CFG0_INT_POL_ACTIVE_HIGH BIT(2)
> +#define OPT4048_CFG0_LATCH BIT(3)
> +#define OPT4048_CFG0_OP_MODE_POWER_DOWN 0
> +#define OPT4048_CFG0_OP_MODE_ONESHOT_AUTORANGE (1 << 4)
> +#define OPT4048_CFG0_OP_MODE_ONESHOT (2 << 4)
> +#define OPT4048_CFG0_OP_MODE_CONTINUOUS (3 << 4)
> +#define OPT4048_CFG0_CONVERSION_TIME(v) (((v) << 6) & GENMASK(9, 6))
> +#define OPT4048_CFG0_RANGE(v) (((v) << 10) & GENMASK(13, 10))
> +#define OPT4048_CFG0_RANGE_AUTO (12 << 10)
> +#define OPT4048_CFG0_QWAKE BIT(15)
> +
> +#define OPT4048_CFG1 0xb
> +#define OPT4048_CFG1_I2C_BURST BIT(0)
> +#define OPT4048_CFG1_INT_CFG_ALERT 0
> +#define OPT4048_CFG1_INT_CFG_DATA_READY_NEXT (1 << 2)
> +#define OPT4048_CFG1_INT_CFG_DATA_READY_ALL (3 << 2)
> +#define OPT4048_CFG1_INT_DIR_IN 0
> +#define OPT4048_CFG1_INT_DIR_OUT BIT(4)
> +#define OPT4048_CFG1_THRESHOLD_CH_SEL(i) (((i) << 5) & GENMASK(6, 5))
> +#define OPT4048_CFG1_RESERVED (0x80 << 7)
> +
> +#define OPT4048_STATUS 0xc
> +#define OPT4048_STATUS_FLAG_L BIT(0)
> +#define OPT4048_STATUS_FLAG_H BIT(1)
> +#define OPT4048_STATUS_CONV_READY_FLAG BIT(2)
> +#define OPT4048_STATUS_OVERLOAD_FLAG BIT(3)
> +
> +#define OPT4048_DID 0x11
> +#define OPT4048_DID_L_VALUE(v) (((v) & GENMASK(13, 12)) >> 12)
> +#define OPT4048_DID_H_VALUE(v) ((v) & GENMASK(11, 0))
Just define the masks and use FIELD_GET() to extract the fields.
> +#define OPT4048_DID_VALUE(l, h) (((h) << 2) | (l))
Use FIELD_PREP for this.
> +
> +#define OPT4048_DID_OPT4048 0x2084
> +
> +#define OPT4048_SCALE_ULUX 2150
> +
> +struct opt4048_sensor_state {
> + bool active;
> + u8 conversion_time_index;
Whilst I don't normally care that much, this is really bad for packing.
Put like types together.
> + u16 threshold_low[2];
> + bool threshold_low_active;
> + u16 threshold_high[2];
> + bool threshold_high_active;
> + u16 status;
> +};
> +
> +struct opt4048_sensor_scan {
> + u32 channels[5];
> + s64 timestamp __aligned(8);
aligned_s64 is now available upstream.
> +};
> +
> +struct opt4048_sensor {
> + struct device *dev;
> + struct i2c_client *i2c_client;
> + struct iio_dev *iio_dev;
> +
> + struct regulator *vdd_supply;
> +
> + struct opt4048_sensor_state state;
> + struct opt4048_sensor_scan scan;
As below. I think you should just always do the read in the trigger handler.
> + bool scan_sync;
This needs a comment. Interrupts and sync aren't something that normally goes
together so the name is somewhat confusing.
> + struct mutex lock;
Locks should always have a comment to say what data they are protecting.
> +};
> +
> +static int opt4048_data_read(struct opt4048_sensor *sensor, u8 address,
> + u32 *channel_value)
> +{
> + struct i2c_client *i2c_client = sensor->i2c_client;
> + u16 value, result_msb, result_lsb, exponent;
> + unsigned int index = 0;
> + u8 values[4] = { 0 };
> + int ret;
> +
> + /* Read all values in one transaction to ensure coherency. */
> + ret = i2c_smbus_read_i2c_block_data(i2c_client, address, 4, values);
> + if (ret < 0)
> + return ret;
> +
> + value = values[index] << 8 | values[index + 1];
As below. Use a __be16 and suitable conversion funcitons.
> + index += 2;
> +
> + result_msb = OPT4048_CH_DATA0_MSB_VALUE(value);
> + exponent = OPT4048_CH_DATA0_EXPONENT_VALUE(value);
> +
> + value = values[index] << 8 | values[index + 1];
> +
> + result_lsb = (u16)OPT4048_CH_DATA1_LSB_VALUE(value);
> +
> + *channel_value = opt4048_data_value(result_msb, result_lsb, exponent);
> +
> + return 0;
> +}
> +
> +static int opt4048_data_scan(struct opt4048_sensor *sensor,
> + struct opt4048_sensor_scan *scan)
> +{
> + struct i2c_client *i2c_client = sensor->i2c_client;
> + unsigned int index = 0;
> + u8 values[16] = { 0 };
Looks like an array of __be16. Cleaner to define it like that.
> + unsigned int i;
> + int ret;
> +
> + /* Read all values in one transaction to ensure coherency. */
> + ret = i2c_smbus_read_i2c_block_data(i2c_client, OPT4048_CH0_DATA0, 16,
> + values);
> + if (ret < 0)
> + return ret;
> +
> + for (i = 0; i < 4; i++) {
> + u16 value, result_msb, result_lsb, exponent;
> +
> + value = values[index] << 8 | values[index + 1];
get_unaligned_be16()
or better still make values an array of __be16 and use
be16_to_cpu()
> + index += 2;
> +
> + result_msb = (u16)OPT4048_CH_DATA0_MSB_VALUE(value);
> + exponent = (u16)OPT4048_CH_DATA0_EXPONENT_VALUE(value);
Use FIELD_GET() and suitable masks for this.
> +
> + value = values[index] << 8 | values[index + 1];
> + index += 2;
> +
as above..
> + result_lsb = (u16)OPT4048_CH_DATA1_LSB_VALUE(value);
> +
> + scan->channels[i] =
> + opt4048_data_value(result_msb, result_lsb, exponent);
> + }
> +
> + /* Report illuminance using Y intensity value. */
That seems 'interesting'. Why?
> + scan->channels[4] = scan->channels[1];
> +
> + return 0;
> +}
> +
> +static int opt4048_identify(struct opt4048_sensor *sensor)
> +{
> + struct device *dev = sensor->dev;
> + int ret;
> + u16 id, low, high;
> +
> + ret = pm_runtime_resume_and_get(dev);
> + if (ret < 0)
> + return ret;
> +
> + ret = i2c_smbus_read_word_swapped(sensor->i2c_client, OPT4048_DID);
> + if (ret < 0)
> + goto complete;
> +
> + low = (u16)OPT4048_DID_L_VALUE(ret);
> + high = (u16)OPT4048_DID_H_VALUE(ret);
> +
> + id = OPT4048_DID_VALUE(low, high);
> +
> + switch (id) {
> + case OPT4048_DID_OPT4048:
> + dev_info(dev, "identified OPT4048 sensor\n");
This isn't useful information and can be easily established once the
driver has loaded. So dev_dbg() at most.
> + ret = 0;
> + break;
> + default:
> + dev_err(dev, "unknown sensor with id: %#x\n", id);
> + ret = -ENODEV;
We shouldn't treat a failure to match the ID as a reason to fail probe.
Consider the use of fallback IDs in device tree with older kernels.
If a new device comes along that is backwards compatible, we want that
to work with out driver modifications.
It's fine to print an dev_info message though so the user is aware
that we are ignoring the missmatch.
> + break;
> + }
> +
> +complete:
> + pm_runtime_put_sync(dev);
> +
> + return ret;
> +}
> +
> +static int opt4048_power(struct opt4048_sensor *sensor, bool on)
> +{
> + struct opt4048_sensor_state *state = &sensor->state;
> + int ret;
> +
> + mutex_lock(&sensor->lock);
guard()
> +
> + state->active = on;
> + ret = opt4048_state_configure_cfg0(sensor);
> +
> + mutex_unlock(&sensor->lock);
> +
> + return ret;
> +}
> +static int opt4048_state_configure_cfg1(struct opt4048_sensor *sensor)
> +{
> + u16 value;
> + int ret;
I don't really see any advantage in these wrappers. I'd just have
the code inline in opt4048_state_configure()
> +
> + /* Assign threshold to the Y channel for illuminance. */
> + value = OPT4048_CFG1_I2C_BURST |
> + OPT4048_CFG1_INT_DIR_OUT |
> + OPT4048_CFG1_THRESHOLD_CH_SEL(1) |
> + OPT4048_CFG1_RESERVED;
combine these on fewer lines. Just generally stay below 80 chars unless
there is a strong readability reason to go longer.
> +
> + if (sensor->scan_sync)
> + value |= OPT4048_CFG1_INT_CFG_DATA_READY_ALL;
> + else
> + value |= OPT4048_CFG1_INT_CFG_ALERT;
> +
> + ret = i2c_smbus_write_word_swapped(sensor->i2c_client, OPT4048_CFG1,
> + value);
> + if (ret < 0)
> + return ret;
> +
> + return 0;
> +}
> +
> +static int opt4048_state_configure(struct opt4048_sensor *sensor)
> +{
> + int ret;
> +
> + mutex_lock(&sensor->lock);
guard() See below.
> +
> + ret = opt4048_state_configure_cfg0(sensor);
> + if (ret)
> + goto complete;
> +
> + ret = opt4048_state_configure_cfg1(sensor);
> + if (ret)
> + goto complete;
> +
> + ret = opt4048_state_configure_threshold(sensor);
> + if (ret)
> + goto complete;
> +
> +complete:
> + mutex_unlock(&sensor->lock);
> +
> + return ret;
> +}
> +
> +static void opt4048_state_reset(struct opt4048_sensor *sensor)
> +{
> + struct opt4048_sensor_state *state = &sensor->state;
> +
> + state->active = false;
> +
> + /* Start with a 25 ms integration time. */
> + state->conversion_time_index = 6;
> +
> + state->threshold_low_active = false;
> + state->threshold_high_active = false;
> +}
> +
> +/* IIO */
> +
> +static const struct iio_event_spec opt4048_iio_events[] = {
> + {
> + .type = IIO_EV_TYPE_THRESH,
> + .dir = IIO_EV_DIR_RISING,
> + .mask_separate =
> + BIT(IIO_EV_INFO_ENABLE) |
> + BIT(IIO_EV_INFO_VALUE),
> + },
> + {
> + .type = IIO_EV_TYPE_THRESH,
> + .dir = IIO_EV_DIR_FALLING,
> + .mask_separate =
> + BIT(IIO_EV_INFO_ENABLE) |
> + BIT(IIO_EV_INFO_VALUE),
> + },
> +};
> +
> +static const struct iio_chan_spec opt4048_iio_channels[] = {
> + {
> + .type = IIO_INTENSITY,
> + .channel2 = IIO_MOD_X,
> + .address = OPT4048_CH0_DATA0,
> + .modified = 1,
> +
> + .scan_index = 0,
> + .scan_type = {
> + .sign = 'u',
> + .realbits = 28,
> + .storagebits = 32,
> + .endianness = IIO_LE,
> + },
> +
> + .info_mask_separate =
> + BIT(IIO_CHAN_INFO_RAW),
.info_mask_separate = BIT(IIO_CHAN_INFO_RAW),
still a short line. So don't wrap too aggressively.
> + .info_mask_shared_by_all =
> + BIT(IIO_CHAN_INFO_INT_TIME),
> + .info_mask_shared_by_all_available =
> + BIT(IIO_CHAN_INFO_INT_TIME),
> + },
> + {
> + .type = IIO_INTENSITY,
> + .channel2 = IIO_MOD_Y,
> + .address = OPT4048_CH1_DATA0,
> + .modified = 1,
> +
> + .scan_index = 1,
> + .scan_type = {
> + .sign = 'u',
> + .realbits = 28,
> + .storagebits = 32,
> + .endianness = IIO_LE,
> + },
> +
> + .info_mask_separate =
> + BIT(IIO_CHAN_INFO_RAW),
> + .info_mask_shared_by_all =
> + BIT(IIO_CHAN_INFO_INT_TIME),
> + .info_mask_shared_by_all_available =
> + BIT(IIO_CHAN_INFO_INT_TIME),
> + },
> + {
> + .type = IIO_INTENSITY,
> + .channel2 = IIO_MOD_Z,
> + .address = OPT4048_CH2_DATA0,
> + .modified = 1,
> +
> + .scan_index = 2,
> + .scan_type = {
> + .sign = 'u',
> + .realbits = 28,
> + .storagebits = 32,
> + .endianness = IIO_LE,
> + },
> +
> + .info_mask_separate =
> + BIT(IIO_CHAN_INFO_RAW),
> + .info_mask_shared_by_all =
> + BIT(IIO_CHAN_INFO_INT_TIME),
> + .info_mask_shared_by_all_available =
> + BIT(IIO_CHAN_INFO_INT_TIME),
> + },
> + {
> + .type = IIO_INTENSITY,
> + .channel2 = IIO_MOD_LIGHT_BOTH,
> + .address = OPT4048_CH3_DATA0,
> + .modified = 1,
> +
> + .scan_index = 3,
> + .scan_type = {
> + .sign = 'u',
> + .realbits = 28,
> + .storagebits = 32,
> + .endianness = IIO_LE,
> + },
> +
> + .info_mask_separate =
> + BIT(IIO_CHAN_INFO_RAW),
> + .info_mask_shared_by_all =
> + BIT(IIO_CHAN_INFO_INT_TIME),
> + .info_mask_shared_by_all_available =
> + BIT(IIO_CHAN_INFO_INT_TIME),
> + },
> + {
> + .type = IIO_LIGHT,
> + .address = OPT4048_CH1_DATA0,
> +
> + .scan_index = 4,
> + .scan_type = {
> + .sign = 'u',
> + .realbits = 28,
> + .storagebits = 32,
> + .endianness = IIO_LE,
> + },
> +
> + .info_mask_separate =
> + BIT(IIO_CHAN_INFO_RAW) |
> + BIT(IIO_CHAN_INFO_SCALE),
> + .info_mask_shared_by_all =
> + BIT(IIO_CHAN_INFO_INT_TIME),
> + .info_mask_shared_by_all_available =
> + BIT(IIO_CHAN_INFO_INT_TIME),
> + },
> + IIO_CHAN_SOFT_TIMESTAMP(5),
> +};
> +
> +static int opt4048_iio_read_raw(struct iio_dev *iio_dev,
> + struct iio_chan_spec const *channel,
> + int *value_first, int *value_second, long mask)
> +{
> + struct opt4048_sensor *sensor = iio_priv(iio_dev);
> + struct opt4048_sensor_state *state = &sensor->state;
> + struct device *dev = sensor->dev;
> + int ret;
> +
> + switch (mask) {
> + case IIO_CHAN_INFO_RAW:
Need define scope {}
> + unsigned int scan_index;
> +
> + ret = iio_device_claim_direct_mode(iio_dev);
> + if (ret)
> + return ret;
> +
> + ret = pm_runtime_resume_and_get(dev);
> + if (ret < 0)
> + goto release_direct_mode;
> +
> + if (sensor->scan_sync) {
So this is curious. You just power up sensor and let it run until interrupt.
I'd expect to have interrupt disabled unless buffered capture is in use.
It would be fine to turn it on for a single cycle though if that makes capturing
on demand simpler.
> + scan_index = channel->scan_index;
> +
> + mutex_lock(&sensor->lock);
> + *value_first = (int)sensor->scan.channels[scan_index];
> + mutex_unlock(&sensor->lock);
> + } else {
> + ret = opt4048_data_read(sensor, channel->address,
> + (u32 *)value_first);
> + }
> +
> + pm_runtime_mark_last_busy(dev);
> + pm_runtime_put_autosuspend(dev);
> +
> +release_direct_mode:
I'd factor out the code above into a another function, then can avoid
the ugly goto within a switch statement.
> + iio_device_release_direct_mode(iio_dev);
> +
> + if (ret < 0)
> + return ret;
> +
> + return IIO_VAL_INT;
> +
> + case IIO_CHAN_INFO_SCALE:
> + *value_first = 0;
> + *value_second = OPT4048_SCALE_ULUX;
> +
> + return IIO_VAL_INT_PLUS_MICRO;
> +
> + case IIO_CHAN_INFO_INT_TIME:
> + unsigned int index = 2 * state->conversion_time_index;
> +
> + *value_first = opt4048_conversion_time_available[index];
> + index++;
> + *value_second = opt4048_conversion_time_available[index];
> +
> + return IIO_VAL_INT_PLUS_MICRO;
> +
> + default:
> + return -EINVAL;
> + }
> +}
> +
> +static int opt4048_iio_write_event_value(struct iio_dev *iio_dev,
> + struct iio_chan_spec const *channel,
> + enum iio_event_type type,
> + enum iio_event_direction direction,
> + enum iio_event_info info,
> + int value_first, int value_second)
> +{
> + struct opt4048_sensor *sensor = iio_priv(iio_dev);
> + struct opt4048_sensor_state *state = &sensor->state;
> + u32 value;
> + int ret;
> +
> + switch (direction) {
> + case IIO_EV_DIR_RISING:
> + value = (u32)value_first;
> + opt4048_threshold_convert(value, state->threshold_high);
> + break;
> +
> + case IIO_EV_DIR_FALLING:
> + value = (u32)value_first;
> + opt4048_threshold_convert(value, state->threshold_low);
> + break;
> +
> + default:
> + return -EINVAL;
> + }
> +
> + if (pm_runtime_suspended(sensor->dev))
> + return 0;
I'm curious. Why isn't this an error return? Check for other cases of this.
I think they should all return an error so userspace knows something unexpected
is going on.
> +
> + ret = opt4048_state_configure_threshold(sensor);
> + if (ret)
> + return ret;
return opt...
> +
> + return 0;
> +}
> +
> +static int opt4048_iio_write_event_config(struct iio_dev *iio_dev,
> + struct iio_chan_spec const *channel,
> + enum iio_event_type type,
> + enum iio_event_direction direction,
> + int active)
Now takes a bool. Rebase on either my iio.git / testing branch or
rc1 once available (probably later today).
> +{
> + struct opt4048_sensor *sensor = iio_priv(iio_dev);
> + struct opt4048_sensor_state *state = &sensor->state;
> + int ret = 0;
> +
> + /* Threshold active are read in IRQ thread. */
> + mutex_lock(&sensor->lock);
guard()
> +
> + switch (direction) {
> + case IIO_EV_DIR_RISING:
> + state->threshold_high_active = !!active;
This is why the signature became bool. All drivers had the same handling
which made no sense :(
FWIW you don't need the !! even before that change.
> + break;
> +
> + case IIO_EV_DIR_FALLING:
> + state->threshold_low_active = !!active;
> + break;
> +
> + default:
> + ret = -EINVAL;
When using guard. Can just return -EINVAL;
> + goto complete;
> + }
> +
> + if (pm_runtime_suspended(sensor->dev))
> + goto complete;
As above - direct return.
Though fun question of what you should do if this does fail as
we are then in a weird unknown state.
why not an error return?
> +
> + ret = opt4048_state_configure_threshold(sensor);
Will become
return opt4048_*
> +
> +complete:
> + mutex_unlock(&sensor->lock);
> +
> + return ret;
> +}
};
> +
> +static irqreturn_t opt4048_iio_buffer_trigger(int irq, void *data)
> +{
> + struct iio_poll_func *poll_func = data;
> + struct iio_dev *iio_dev = poll_func->indio_dev;
> + struct opt4048_sensor *sensor = iio_priv(iio_dev);
> + struct opt4048_sensor_scan scan = { 0 };
> + s64 timestamp;
> + unsigned int index = 0;
> + unsigned int i;
> + int ret;
> +
> + /* Capture timestamp just before reading values. */
> + timestamp = iio_get_time_ns(iio_dev);
> +
> + mutex_lock(&sensor->lock);
> +
> + if (!sensor->scan_sync) {
> + ret = opt4048_data_scan(sensor, &sensor->scan);
So you have a weird hybrid of capture in the data ready interrupt and here.
Why not just kick this off by having a data ready trigger and
using iio_trigger_poll() to effectively call this on the data ready
interrupt or on an other trigger. That way should need no special handling
for your sync scan.
> + if (ret)
> + goto complete;
> + }
> +
> + for_each_set_bit(i, iio_dev->active_scan_mask, iio_dev->masklength) {
> + /* Assume scan index matches array index. */
> + const struct iio_chan_spec *channel = &opt4048_iio_channels[i];
> + unsigned int scan_index = channel->scan_index;
> +
> + /* Only active channels are reported, in order. */
> + scan.channels[index] = sensor->scan.channels[scan_index];
> + index++;
If you are always reading all channels, set avail_scan_masks and let the
demux in the IIO core deal with this data rearranging.
> + }
> +
> + iio_push_to_buffers_with_timestamp(iio_dev, &scan, timestamp);
> +
> +complete:
> + mutex_unlock(&sensor->lock);
> +
> + iio_trigger_notify_done(iio_dev->trig);
> +
> + return IRQ_HANDLED;
> +}
...
> +static const struct iio_buffer_setup_ops opt4048_iio_buffer_setup_ops = {
> + .preenable = opt4048_iio_buffer_preenable,
> + .postdisable = opt4048_iio_buffer_postdisable,
> +};
> +
> +static int opt4048_iio_setup(struct opt4048_sensor *sensor)
This function doesn't add much. I'd just put the code inline in prbe.
> +{
> + struct iio_chan_spec *channels;
> + struct iio_dev *iio_dev = sensor->iio_dev;
Just pass it the iio_dev in.
> + struct device *dev = sensor->dev;
> + unsigned int channels_count;
> + int ret;
> +
> + channels_count = ARRAY_SIZE(opt4048_iio_channels);
> +
> + if (sensor->i2c_client->irq > 0) {
> + channels = devm_kzalloc(dev, sizeof(opt4048_iio_channels),
> + GFP_KERNEL);
kmemdup. However I'd rather just see this picking between two static
const arrays of channels. There are only two cases so it isn't worth dynamic
channel setup complexity.
> +
> + memcpy(channels, opt4048_iio_channels,
> + sizeof(opt4048_iio_channels));
> +
> + /* Attach threshold events to the illuminance channel. */
> + channels[3].event_spec = opt4048_iio_events;
> + channels[3].num_event_specs = ARRAY_SIZE(opt4048_iio_events);
> + } else {
> + /* Threshold events are not available without an irq. */
> + channels = (struct iio_chan_spec *)opt4048_iio_channels;
> + }
> +
> + iio_dev->info = &opt4048_iio_info;
> + iio_dev->name = "opt4048";
> + iio_dev->channels = channels;
> + iio_dev->num_channels = channels_count;
> + iio_dev->modes = INDIO_DIRECT_MODE;
> +
> + ret = devm_iio_triggered_buffer_setup(dev, iio_dev, NULL,
> + opt4048_iio_buffer_trigger,
> + &opt4048_iio_buffer_setup_ops);
> + if (ret) {
> + dev_err(dev, "failed to setup iio triggered buffer\n");
> + return ret;
> + }
> +
> + ret = iio_device_register(iio_dev);
As below. No reason not to use devm_iio_device_register() and avoid
need to manually unwind.
> + if (ret) {
> + dev_err(dev, "failed to register iio device\n");
> + return ret;
> + }
> +
> + return 0;
> +}
> +
> +static void opt4048_iio_cleanup(struct opt4048_sensor *sensor)
> +{
> + iio_device_unregister(sensor->iio_dev);
This wrapper doesn't add anything useful so put the code inline.
Though as below, it shouldn't be called directly at all.
> +}
> +
> +/* IRQ */
Comment doesn't add anything that isn't obvious from the code.
So don't have it.
> +
> +static irqreturn_t opt4048_irq(int irq, void *data)
> +{
> + struct opt4048_sensor *sensor = data;
> + struct opt4048_sensor_state *state = &sensor->state;
> + struct iio_dev *iio_dev = sensor->iio_dev;
Set data = iio_dev and then use iio_priv() on that.
There shouldn't be a need to go back the other way.
> + bool threshold_rising;
> + bool threshold_falling;
> + s64 timestamp;
> + u64 code;
Might as well combine elements of same type. Saves a few lines of code
and doesn't harm readability much.
> + u16 status;
> + int ret;
> +
> + timestamp = iio_get_time_ns(iio_dev);
> +
> + ret = i2c_smbus_read_word_swapped(sensor->i2c_client, OPT4048_STATUS);
> + if (ret < 0)
> + return IRQ_HANDLED;
> +
> + status = (u16)ret;
> +
> + mutex_lock(&sensor->lock);
Use
guard(mutex)(&sensor->lock);
and include cleanup.h
That will simplify this function as you can then do direct returns
instead of gotos.
> +
> + if (!state->active)
Perhaps add a comment for this. I guess it's about preventing a race?
+ the lock prevents concurrent power down.
I'd be tempted to just do a pm_runtime_resume_and_get()
Vast majority of the time the device will be powered up and you won't
need to do anything. The race will be closed as in the rare corner
case w will be able ot read the data.
I'm also curious that you can still read status after powering down.
Maybe add a comment about that always being safe.
> + goto complete;
> +
> + if (status & OPT4048_STATUS_CONV_READY_FLAG)
> + opt4048_data_scan(sensor, &sensor->scan);
Superficially this looks like a data ready trigger should be used.
So register a trigger and do the read in the trigger handler in all
cases rather than here.
> +
> + threshold_falling = (status & OPT4048_STATUS_FLAG_L) &&
> + !(state->status & OPT4048_STATUS_FLAG_L);
> +
> + if (state->threshold_low_active && threshold_falling) {
> + code = IIO_UNMOD_EVENT_CODE(IIO_LIGHT, 3, IIO_EV_TYPE_THRESH,
> + IIO_EV_DIR_FALLING);
> + iio_push_event(iio_dev, code, timestamp);
> + }
> +
> + threshold_rising = (status & OPT4048_STATUS_FLAG_H) &&
> + !(state->status & OPT4048_STATUS_FLAG_H);
> +
> + if (state->threshold_high_active && threshold_rising) {
> + code = IIO_UNMOD_EVENT_CODE(IIO_LIGHT, 3, IIO_EV_TYPE_THRESH,
> + IIO_EV_DIR_RISING);
> + iio_push_event(iio_dev, code, timestamp);
> + }
> +
> + state->status = status;
Add a comment on why you are saving this (I guess because the device
interrupts every time otherwise). Does it interrupt on the reverse
direction? If not how is this cleared?
> +
> +complete:
> + mutex_unlock(&sensor->lock);
> +
> + return IRQ_HANDLED;
> +}
> +
> +/* PM */
Structural comments like this don't add much after a driver is
written and often become wrong as a driver develops over time.
I'd drop them.
> +
> +static int opt4048_suspend(struct device *dev)
> +{
> + struct opt4048_sensor *sensor = dev_get_drvdata(dev);
> + int ret;
> +
> + ret = opt4048_power(sensor, 0);
> + if (ret)
> + goto error;
> +
> + ret = regulator_disable(sensor->vdd_supply);
> + if (ret)
> + goto error;
> +
> + return 0;
> +
> +error:
> + return -EAGAIN;
Direct returns preferred over both eating the error codes and
a goto like this.
> +}
> +
> +static int opt4048_resume(struct device *dev)
> +{
> + struct opt4048_sensor *sensor = dev_get_drvdata(dev);
> + unsigned long sleep_min;
> + unsigned int index;
> + int ret;
> +
> + ret = regulator_enable(sensor->vdd_supply);
> + if (ret)
> + goto error;
> +
> + /* Wait for the regulator to settle and the chip to power-on. */
> + udelay(30);
> +
> + ret = opt4048_state_configure(sensor);
> + if (ret)
> + goto error_regulator;
> +
> + ret = opt4048_power(sensor, 1);
> + if (ret)
> + goto error_regulator;
> +
> + /* Wait for conversion to be ready for all channels. */
We might not have powered up for that reason but I guess this does
little harm.
> + index = 2 * sensor->state.conversion_time_index + 1;
> + sleep_min = opt4048_conversion_time_available[index] * 4;
> +
> + usleep_range(sleep_min, 5 * sleep_min / 4);
Perhaps use fsleep() to avoid anyone needing to reason about the margins
etc.
> +
> + return 0;
> +
> +error_regulator:
> + regulator_disable(sensor->vdd_supply);
> +
> +error:
Direct returns make for easier to review code as following an error
path doesn't require checking to see what the cleanup is.
> + return -EAGAIN;
> +}
> +
> +static const struct dev_pm_ops opt4048_pm_ops = {
> + .runtime_suspend = opt4048_suspend,
> + .runtime_resume = opt4048_resume,
> +};
> +
> +/* I2C */
Not a particularly useful comment. I'd drop it.
> +
> +static int opt4048_probe(struct i2c_client *client)
> +{
> + struct device *dev = &client->dev;
> + struct opt4048_sensor *sensor;
> + struct iio_dev *iio_dev;
> + int irq = client->irq;
> + int ret;
> +
> + iio_dev = devm_iio_device_alloc(dev, sizeof(*sensor));
> + if (!iio_dev)
> + return -ENOMEM;
> +
> + sensor = iio_priv(iio_dev);
> +
> + sensor->dev = dev;
> + sensor->i2c_client = client;
> + sensor->iio_dev = iio_dev;
That is almost always a sign that you have a less than ideal layering
in the driver.
> +
> + i2c_set_clientdata(client, sensor);
> +
> + mutex_init(&sensor->lock);
ret = devm_mutex_init()
if (ret)
return ret;
> +
> + sensor->vdd_supply = devm_regulator_get(dev, "vdd");
Given runtime PM may not even be enabled, you should turn the power on.
Unconditionally.
> + if (IS_ERR(sensor->vdd_supply)) {
> + dev_err(dev, "failed to get VDD regulator\n");
For error returns and messages in probe use return dev_err_probe()
It will do various useful things for deferred and out of memory
error prints. Find to use it in all code only called from the probe
callback.
> + return PTR_ERR(sensor->vdd_supply);
> + }
> +
> + opt4048_state_reset(sensor);
> +
> + devm_pm_runtime_enable(dev);
Can fail so check for that.
> +
> + ret = opt4048_identify(sensor);
> + if (ret)
As above, failure to match should not be a reason to fail probe.
> + return ret;
> +
> + pm_runtime_set_autosuspend_delay(dev, 5000);
> + pm_runtime_use_autosuspend(dev);
> +
> + if (irq > 0) {
> + ret = devm_request_threaded_irq(dev, irq, NULL, opt4048_irq,
> + IRQF_TRIGGER_FALLING |
> + IRQF_ONESHOT,
Direction should come from firmware (so don't specify it here).
It's reasonably common to find an inverter in the path for an interrupt
and so we can't know the sense in the driver.
> + "opt4048", sensor);
> + if (ret) {
> + dev_err(dev, "failed to request irq\n");
> + return ret;
> + }
> +
> + sensor->scan_sync = true;
> + }
> +
> + ret = opt4048_iio_setup(sensor);
As above, I'd put the contents of that function here as well. Whilst
I guess you wanted to keep functions small, sometimes that just leads
to less readable code than a long function with everything we expect
in it.
> + if (ret)
> + return ret;
> +
> + return 0;
> +}
> +
> +static void opt4048_remove(struct i2c_client *client)
> +{
> + struct opt4048_sensor *sensor = i2c_get_clientdata(client);
> +
> + opt4048_iio_cleanup(sensor);
For a simple single call like this, use a devm_add_action_or_reset()
so that the cleanup is associated directly with the setup function and
we don't need a remove() callback at all.
Given all it does is iio_device_unregister() just use devm_device_register()
in the first place and drop this.
> +}
> +
> +static const struct i2c_device_id opt4048_id[] = {
> + { "opt4048" },
> + { }
> +};
> +
> +MODULE_DEVICE_TABLE(i2c, opt4048_id);
> +
> +static const struct of_device_id opt4048_of_match[] = {
> + { .compatible = "ti,opt4048" },
> + { }
> +};
Similar to below, fairly standard to group the id table and
MODULE_DEVICE_TABLE but not having a blank line.
> +
> +MODULE_DEVICE_TABLE(of, opt4048_of_match);
> +
> +static struct i2c_driver opt4048_driver = {
> + .driver = {
> + .name = "opt4048",
> + .of_match_table = opt4048_of_match,
> + .pm = &opt4048_pm_ops,
> + },
> + .id_table = opt4048_id,
> + .probe = opt4048_probe,
> + .remove = opt4048_remove,
> +};
> +
It's common to not put a blank line here given how closely coupled this
macro is to the structure.
> +module_i2c_driver(opt4048_driver);
> +
> +MODULE_AUTHOR("Paul Kocialkowski <paulk@sys-base.io>");
> +MODULE_DESCRIPTION("Texas Instruments OPT4048 XYZ tristimulus color sensor driver");
> +MODULE_LICENSE("GPL");
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 2/2] iio: light: Add support for the TI OPT4048 color sensor
2024-12-01 11:55 ` Jonathan Cameron
@ 2024-12-01 17:56 ` Paul Kocialkowski
2024-12-02 11:06 ` Jonathan Cameron
0 siblings, 1 reply; 10+ messages in thread
From: Paul Kocialkowski @ 2024-12-01 17:56 UTC (permalink / raw)
To: Jonathan Cameron
Cc: linux-iio, devicetree, linux-kernel, Lars-Peter Clausen,
Rob Herring, Krzysztof Kozlowski, Conor Dooley
[-- Attachment #1: Type: text/plain, Size: 40578 bytes --]
Hi Jonathan,
Le Sun 01 Dec 24, 11:55, Jonathan Cameron a écrit :
> On Sat, 30 Nov 2024 18:42:12 +0100
> Paul Kocialkowski <paulk@sys-base.io> wrote:
>
> > The Texas Instruments OPT4048 is a XYZ tristimulus color sensor,
> > with an additional wide (visible + IR) channel.
> >
> > This driver implements support for all channels, with configurable
> > integration time and auto-gain. Both direct reading and
> > triggered-buffer modes are supported.
> >
> > Note that the Y channel is also reported as a separate illuminance
> > channel, for which a scale is provided (following the datasheet) to
> > convert it to lux units. Falling and rising thresholds are supported
> > for this channel.
> >
> > The device's interrupt can be used to sample all channels at the end
> > of conversion and is optional.
> >
> > Signed-off-by: Paul Kocialkowski <paulk@sys-base.io>
> Hi Paul,
>
> Various comments inline. Most significant is that this seems to be
> suitable for a simple dataready trigger that will make your various
> interrupt and non interrupt flows more similar.
And thanks for the fast review and insightful comments!
I considered implementing a trigger in the driver, but the issue I found
is that the trigger is expected to be called from hard irq context,
while the new values are read in the bottom half. I understand the triggered
buffer callbacks are executed as a thread as well, so there would be race
between the two which could result in previous values being returned.
So I concluded that it was more beneficial to preserve the synchronous reading
mechanism over implementing the trigger.
But maybe I missed/misunderstood something here.
> Jonathan
>
> > diff --git a/drivers/iio/light/Makefile b/drivers/iio/light/Makefile
> > index 321010fc0b93..f2031e6236f9 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_OPT4048) += opt4048.o
> > obj-$(CONFIG_PA12203001) += pa12203001.o
> > obj-$(CONFIG_ROHM_BU27008) += rohm-bu27008.o
> > obj-$(CONFIG_ROHM_BU27034) += rohm-bu27034.o
> > diff --git a/drivers/iio/light/opt4048.c b/drivers/iio/light/opt4048.c
> > new file mode 100644
> > index 000000000000..1ad5e6586aad
> > --- /dev/null
> > +++ b/drivers/iio/light/opt4048.c
> > @@ -0,0 +1,1145 @@
> > +// SPDX-License-Identifier: GPL-2.0-only
> > +/*
> > + * Copyright 2024 Paul Kocialkowski <paulk@sys-base.io>
> > + */
> > +
> > +#include <linux/delay.h>
> > +#include <linux/i2c.h>
> > +#include <linux/iio/events.h>
> > +#include <linux/iio/iio.h>
> > +#include <linux/iio/trigger_consumer.h>
> > +#include <linux/iio/triggered_buffer.h>
> > +#include <linux/log2.h>
> > +#include <linux/module.h>
> > +#include <linux/pm_runtime.h>
> > +#include <linux/regulator/consumer.h>
> > +
> > +#define OPT4048_CH0_DATA0 0x0
> > +#define OPT4048_CH0_DATA1 0x1
> > +#define OPT4048_CH1_DATA0 0x2
> > +#define OPT4048_CH1_DATA1 0x3
> > +#define OPT4048_CH2_DATA0 0x4
> > +#define OPT4048_CH2_DATA1 0x5
> > +#define OPT4048_CH3_DATA0 0x6
> > +#define OPT4048_CH3_DATA1 0x7
> > +
> > +#define OPT4048_CH_DATA0_MSB_VALUE(v) ((v) & GENMASK(11, 0))
> > +#define OPT4048_CH_DATA0_EXPONENT_VALUE(v) (((v) & GENMASK(15, 12)) >> 12)
>
> For all these - just define the masks and use FIELD_GET() / FIELD_PREP()
> inline. Masks should be defined with names that make it clear what they are
> masking and in what registers.
I see. Note that I generally tried to follow the datasheet's terminology for
convenience.
> > +
> > +#define OPT4048_CH_DATA1_CRC_VALUE(v) ((v) & GENMASK(3, 0))
> > +#define OPT4048_CH_DATA1_COUNTER_VALUE(v) (((v) & GENMASK(7, 4)) >> 4)
> > +#define OPT4048_CH_DATA1_LSB_VALUE(v) (((v) & GENMASK(15, 8)) >> 8)
> > +
> > +#define OPT4048_THRESHOLD_L 0x8
> > +#define OPT4048_THRESHOLD_H 0x9
> > +
> > +#define OPT4048_THRESHOLD_RESULT(v) ((v) & GENMASK(11, 0))
> > +#define OPT4048_THRESHOLD_RESULT_MAX ((1 << 12) - 1)
> > +#define OPT4048_THRESHOLD_EXPONENT(v) (((v) << 12) & GENMASK(15, 12))
> > +#define OPT4048_THRESHOLD_EXPONENT_MAX ((1 << 4) - 1)
> > +
> > +#define OPT4048_CFG0 0xa
> > +#define OPT4048_CFG0_FAULT_COUNT_1 0
> > +#define OPT4048_CFG0_FAULT_COUNT_2 1
> > +#define OPT4048_CFG0_FAULT_COUNT_4 2
> > +#define OPT4048_CFG0_FAULT_COUNT_8 3
> > +#define OPT4048_CFG0_INT_POL_ACTIVE_LOW 0
> > +#define OPT4048_CFG0_INT_POL_ACTIVE_HIGH BIT(2)
> > +#define OPT4048_CFG0_LATCH BIT(3)
> > +#define OPT4048_CFG0_OP_MODE_POWER_DOWN 0
> > +#define OPT4048_CFG0_OP_MODE_ONESHOT_AUTORANGE (1 << 4)
> > +#define OPT4048_CFG0_OP_MODE_ONESHOT (2 << 4)
> > +#define OPT4048_CFG0_OP_MODE_CONTINUOUS (3 << 4)
> > +#define OPT4048_CFG0_CONVERSION_TIME(v) (((v) << 6) & GENMASK(9, 6))
> > +#define OPT4048_CFG0_RANGE(v) (((v) << 10) & GENMASK(13, 10))
> > +#define OPT4048_CFG0_RANGE_AUTO (12 << 10)
> > +#define OPT4048_CFG0_QWAKE BIT(15)
> > +
> > +#define OPT4048_CFG1 0xb
> > +#define OPT4048_CFG1_I2C_BURST BIT(0)
> > +#define OPT4048_CFG1_INT_CFG_ALERT 0
> > +#define OPT4048_CFG1_INT_CFG_DATA_READY_NEXT (1 << 2)
> > +#define OPT4048_CFG1_INT_CFG_DATA_READY_ALL (3 << 2)
> > +#define OPT4048_CFG1_INT_DIR_IN 0
> > +#define OPT4048_CFG1_INT_DIR_OUT BIT(4)
> > +#define OPT4048_CFG1_THRESHOLD_CH_SEL(i) (((i) << 5) & GENMASK(6, 5))
> > +#define OPT4048_CFG1_RESERVED (0x80 << 7)
> > +
> > +#define OPT4048_STATUS 0xc
> > +#define OPT4048_STATUS_FLAG_L BIT(0)
> > +#define OPT4048_STATUS_FLAG_H BIT(1)
> > +#define OPT4048_STATUS_CONV_READY_FLAG BIT(2)
> > +#define OPT4048_STATUS_OVERLOAD_FLAG BIT(3)
> > +
> > +#define OPT4048_DID 0x11
> > +#define OPT4048_DID_L_VALUE(v) (((v) & GENMASK(13, 12)) >> 12)
> > +#define OPT4048_DID_H_VALUE(v) ((v) & GENMASK(11, 0))
>
> Just define the masks and use FIELD_GET() to extract the fields.
>
> > +#define OPT4048_DID_VALUE(l, h) (((h) << 2) | (l))
> Use FIELD_PREP for this.
>
> > +
> > +#define OPT4048_DID_OPT4048 0x2084
> > +
> > +#define OPT4048_SCALE_ULUX 2150
> > +
> > +struct opt4048_sensor_state {
> > + bool active;
> > + u8 conversion_time_index;
>
> Whilst I don't normally care that much, this is really bad for packing.
> Put like types together.
Sure.
> > + u16 threshold_low[2];
> > + bool threshold_low_active;
> > + u16 threshold_high[2];
> > + bool threshold_high_active;
> > + u16 status;
> > +};
> > +
> > +struct opt4048_sensor_scan {
> > + u32 channels[5];
> > + s64 timestamp __aligned(8);
>
> aligned_s64 is now available upstream.
Alright.
> > +};
> > +
> > +struct opt4048_sensor {
> > + struct device *dev;
> > + struct i2c_client *i2c_client;
> > + struct iio_dev *iio_dev;
> > +
> > + struct regulator *vdd_supply;
> > +
> > + struct opt4048_sensor_state state;
>
> > + struct opt4048_sensor_scan scan;
>
> As below. I think you should just always do the read in the trigger handler.
As far as I could see the hardware doesn't guarantee that values come from
the same read sequence (even if read as a single i2c transaction). This is why
my approach was to read them as early as possible after the irq is signaled
and cache them here, instead of doing the actual read in the trigger handler
or sysfs read callback (which may have both values from the previous and next
reads).
The only thing the hardware guarantees is that the DATA1 value is latched when
DATA0 is read.
> > + bool scan_sync;
>
> This needs a comment. Interrupts and sync aren't something that normally goes
> together so the name is somewhat confusing.
Okay. The idea is to have a synchronous read where all channels are read as
soon as conversion done is signaled.
> > + struct mutex lock;
>
> Locks should always have a comment to say what data they are protecting.
>
> > +};
>
>
> > +
> > +static int opt4048_data_read(struct opt4048_sensor *sensor, u8 address,
> > + u32 *channel_value)
> > +{
> > + struct i2c_client *i2c_client = sensor->i2c_client;
> > + u16 value, result_msb, result_lsb, exponent;
> > + unsigned int index = 0;
> > + u8 values[4] = { 0 };
> > + int ret;
> > +
> > + /* Read all values in one transaction to ensure coherency. */
> > + ret = i2c_smbus_read_i2c_block_data(i2c_client, address, 4, values);
> > + if (ret < 0)
> > + return ret;
> > +
> > + value = values[index] << 8 | values[index + 1];
> As below. Use a __be16 and suitable conversion funcitons.
> > + index += 2;
> > +
> > + result_msb = OPT4048_CH_DATA0_MSB_VALUE(value);
> > + exponent = OPT4048_CH_DATA0_EXPONENT_VALUE(value);
> > +
> > + value = values[index] << 8 | values[index + 1];
> > +
> > + result_lsb = (u16)OPT4048_CH_DATA1_LSB_VALUE(value);
> > +
> > + *channel_value = opt4048_data_value(result_msb, result_lsb, exponent);
> > +
> > + return 0;
> > +}
> > +
> > +static int opt4048_data_scan(struct opt4048_sensor *sensor,
> > + struct opt4048_sensor_scan *scan)
> > +{
> > + struct i2c_client *i2c_client = sensor->i2c_client;
> > + unsigned int index = 0;
> > + u8 values[16] = { 0 };
>
> Looks like an array of __be16. Cleaner to define it like that.
>
> > + unsigned int i;
> > + int ret;
> > +
> > + /* Read all values in one transaction to ensure coherency. */
> > + ret = i2c_smbus_read_i2c_block_data(i2c_client, OPT4048_CH0_DATA0, 16,
> > + values);
> > + if (ret < 0)
> > + return ret;
> > +
> > + for (i = 0; i < 4; i++) {
> > + u16 value, result_msb, result_lsb, exponent;
> > +
> > + value = values[index] << 8 | values[index + 1];
> get_unaligned_be16()
>
> or better still make values an array of __be16 and use
> be16_to_cpu()
Indeed all that will make things easier.
> > + index += 2;
> > +
> > + result_msb = (u16)OPT4048_CH_DATA0_MSB_VALUE(value);
> > + exponent = (u16)OPT4048_CH_DATA0_EXPONENT_VALUE(value);
>
> Use FIELD_GET() and suitable masks for this.
>
> > +
> > + value = values[index] << 8 | values[index + 1];
> > + index += 2;
> > +
> as above..
>
> > + result_lsb = (u16)OPT4048_CH_DATA1_LSB_VALUE(value);
> > +
> > + scan->channels[i] =
> > + opt4048_data_value(result_msb, result_lsb, exponent);
> > + }
> > +
> > + /* Report illuminance using Y intensity value. */
>
> That seems 'interesting'. Why?
The Y channel is defined by CIE1931 to match the (photopic) illuminance response
curve. The main difference is that Y is unit-less while illuminance is defined
in lux. This specific channel gets an extra scale property to report the
conversion coefficient for lux.
However we still need the Y value to be reported as such in order to be related
to X and Z.
> > + scan->channels[4] = scan->channels[1];
> > +
> > + return 0;
> > +}
> > +
> > +static int opt4048_identify(struct opt4048_sensor *sensor)
> > +{
> > + struct device *dev = sensor->dev;
> > + int ret;
> > + u16 id, low, high;
> > +
> > + ret = pm_runtime_resume_and_get(dev);
> > + if (ret < 0)
> > + return ret;
> > +
> > + ret = i2c_smbus_read_word_swapped(sensor->i2c_client, OPT4048_DID);
> > + if (ret < 0)
> > + goto complete;
> > +
> > + low = (u16)OPT4048_DID_L_VALUE(ret);
> > + high = (u16)OPT4048_DID_H_VALUE(ret);
> > +
> > + id = OPT4048_DID_VALUE(low, high);
> > +
> > + switch (id) {
> > + case OPT4048_DID_OPT4048:
> > + dev_info(dev, "identified OPT4048 sensor\n");
> This isn't useful information and can be easily established once the
> driver has loaded. So dev_dbg() at most.
Honestly I see a bunch of driver doing that (especially camera sensors)
and find it generally useful. But I don't mind getting rid of it.
> > + ret = 0;
> > + break;
> > + default:
> > + dev_err(dev, "unknown sensor with id: %#x\n", id);
> > + ret = -ENODEV;
> We shouldn't treat a failure to match the ID as a reason to fail probe.
> Consider the use of fallback IDs in device tree with older kernels.
> If a new device comes along that is backwards compatible, we want that
> to work with out driver modifications.
Honestly this is really just a way to check that i2c transaction are working,
that the device is actually there and to report when it's not.
I'm not sure it's such a good idea to assume that other IDs would not be
a sign that something very wrong is going on (typically some other device
connected on the same i2c address). Failing to probe will prevent
misconfiguring another device.
Just adding another case here if a new device is ever introduced doesn't
sound like such a big drawback in comparison.
> It's fine to print an dev_info message though so the user is aware
> that we are ignoring the missmatch.
>
> > + break;
> > + }
> > +
> > +complete:
> > + pm_runtime_put_sync(dev);
> > +
> > + return ret;
> > +}
> > +
> > +static int opt4048_power(struct opt4048_sensor *sensor, bool on)
> > +{
> > + struct opt4048_sensor_state *state = &sensor->state;
> > + int ret;
> > +
> > + mutex_lock(&sensor->lock);
> guard()
> > +
> > + state->active = on;
> > + ret = opt4048_state_configure_cfg0(sensor);
> > +
> > + mutex_unlock(&sensor->lock);
> > +
> > + return ret;
> > +}
>
> > +static int opt4048_state_configure_cfg1(struct opt4048_sensor *sensor)
> > +{
> > + u16 value;
> > + int ret;
> I don't really see any advantage in these wrappers. I'd just have
> the code inline in opt4048_state_configure()
Well I do think they are important to make the code more digest.
> > +
> > + /* Assign threshold to the Y channel for illuminance. */
> > + value = OPT4048_CFG1_I2C_BURST |
> > + OPT4048_CFG1_INT_DIR_OUT |
> > + OPT4048_CFG1_THRESHOLD_CH_SEL(1) |
> > + OPT4048_CFG1_RESERVED;
>
> combine these on fewer lines. Just generally stay below 80 chars unless
> there is a strong readability reason to go longer.
I find it a lot more readable with one item per line.
> > +
> > + if (sensor->scan_sync)
> > + value |= OPT4048_CFG1_INT_CFG_DATA_READY_ALL;
> > + else
> > + value |= OPT4048_CFG1_INT_CFG_ALERT;
> > +
> > + ret = i2c_smbus_write_word_swapped(sensor->i2c_client, OPT4048_CFG1,
> > + value);
> > + if (ret < 0)
> > + return ret;
> > +
> > + return 0;
> > +}
>
> > +
> > +static int opt4048_state_configure(struct opt4048_sensor *sensor)
> > +{
> > + int ret;
> > +
> > + mutex_lock(&sensor->lock);
> guard() See below.
>
> > +
> > + ret = opt4048_state_configure_cfg0(sensor);
> > + if (ret)
> > + goto complete;
> > +
> > + ret = opt4048_state_configure_cfg1(sensor);
> > + if (ret)
> > + goto complete;
> > +
> > + ret = opt4048_state_configure_threshold(sensor);
> > + if (ret)
> > + goto complete;
> > +
> > +complete:
> > + mutex_unlock(&sensor->lock);
> > +
> > + return ret;
> > +}
> > +
> > +static void opt4048_state_reset(struct opt4048_sensor *sensor)
> > +{
> > + struct opt4048_sensor_state *state = &sensor->state;
> > +
> > + state->active = false;
> > +
> > + /* Start with a 25 ms integration time. */
> > + state->conversion_time_index = 6;
> > +
> > + state->threshold_low_active = false;
> > + state->threshold_high_active = false;
> > +}
> > +
> > +/* IIO */
> > +
> > +static const struct iio_event_spec opt4048_iio_events[] = {
> > + {
> > + .type = IIO_EV_TYPE_THRESH,
> > + .dir = IIO_EV_DIR_RISING,
> > + .mask_separate =
> > + BIT(IIO_EV_INFO_ENABLE) |
> > + BIT(IIO_EV_INFO_VALUE),
> > + },
> > + {
> > + .type = IIO_EV_TYPE_THRESH,
> > + .dir = IIO_EV_DIR_FALLING,
> > + .mask_separate =
> > + BIT(IIO_EV_INFO_ENABLE) |
> > + BIT(IIO_EV_INFO_VALUE),
> > + },
> > +};
> > +
> > +static const struct iio_chan_spec opt4048_iio_channels[] = {
> > + {
> > + .type = IIO_INTENSITY,
> > + .channel2 = IIO_MOD_X,
> > + .address = OPT4048_CH0_DATA0,
> > + .modified = 1,
> > +
> > + .scan_index = 0,
> > + .scan_type = {
> > + .sign = 'u',
> > + .realbits = 28,
> > + .storagebits = 32,
> > + .endianness = IIO_LE,
> > + },
> > +
> > + .info_mask_separate =
> > + BIT(IIO_CHAN_INFO_RAW),
> .info_mask_separate = BIT(IIO_CHAN_INFO_RAW),
>
> still a short line. So don't wrap too aggressively.
>
>
> > + .info_mask_shared_by_all =
> > + BIT(IIO_CHAN_INFO_INT_TIME),
> > + .info_mask_shared_by_all_available =
> > + BIT(IIO_CHAN_INFO_INT_TIME),
> > + },
> > + {
> > + .type = IIO_INTENSITY,
> > + .channel2 = IIO_MOD_Y,
> > + .address = OPT4048_CH1_DATA0,
> > + .modified = 1,
> > +
> > + .scan_index = 1,
> > + .scan_type = {
> > + .sign = 'u',
> > + .realbits = 28,
> > + .storagebits = 32,
> > + .endianness = IIO_LE,
> > + },
> > +
> > + .info_mask_separate =
> > + BIT(IIO_CHAN_INFO_RAW),
> > + .info_mask_shared_by_all =
> > + BIT(IIO_CHAN_INFO_INT_TIME),
> > + .info_mask_shared_by_all_available =
> > + BIT(IIO_CHAN_INFO_INT_TIME),
> > + },
> > + {
> > + .type = IIO_INTENSITY,
> > + .channel2 = IIO_MOD_Z,
> > + .address = OPT4048_CH2_DATA0,
> > + .modified = 1,
> > +
> > + .scan_index = 2,
> > + .scan_type = {
> > + .sign = 'u',
> > + .realbits = 28,
> > + .storagebits = 32,
> > + .endianness = IIO_LE,
> > + },
> > +
> > + .info_mask_separate =
> > + BIT(IIO_CHAN_INFO_RAW),
> > + .info_mask_shared_by_all =
> > + BIT(IIO_CHAN_INFO_INT_TIME),
> > + .info_mask_shared_by_all_available =
> > + BIT(IIO_CHAN_INFO_INT_TIME),
> > + },
> > + {
> > + .type = IIO_INTENSITY,
> > + .channel2 = IIO_MOD_LIGHT_BOTH,
> > + .address = OPT4048_CH3_DATA0,
> > + .modified = 1,
> > +
> > + .scan_index = 3,
> > + .scan_type = {
> > + .sign = 'u',
> > + .realbits = 28,
> > + .storagebits = 32,
> > + .endianness = IIO_LE,
> > + },
> > +
> > + .info_mask_separate =
> > + BIT(IIO_CHAN_INFO_RAW),
> > + .info_mask_shared_by_all =
> > + BIT(IIO_CHAN_INFO_INT_TIME),
> > + .info_mask_shared_by_all_available =
> > + BIT(IIO_CHAN_INFO_INT_TIME),
> > + },
> > + {
> > + .type = IIO_LIGHT,
> > + .address = OPT4048_CH1_DATA0,
> > +
> > + .scan_index = 4,
> > + .scan_type = {
> > + .sign = 'u',
> > + .realbits = 28,
> > + .storagebits = 32,
> > + .endianness = IIO_LE,
> > + },
> > +
> > + .info_mask_separate =
> > + BIT(IIO_CHAN_INFO_RAW) |
> > + BIT(IIO_CHAN_INFO_SCALE),
> > + .info_mask_shared_by_all =
> > + BIT(IIO_CHAN_INFO_INT_TIME),
> > + .info_mask_shared_by_all_available =
> > + BIT(IIO_CHAN_INFO_INT_TIME),
> > + },
> > + IIO_CHAN_SOFT_TIMESTAMP(5),
> > +};
> > +
> > +static int opt4048_iio_read_raw(struct iio_dev *iio_dev,
> > + struct iio_chan_spec const *channel,
> > + int *value_first, int *value_second, long mask)
> > +{
> > + struct opt4048_sensor *sensor = iio_priv(iio_dev);
> > + struct opt4048_sensor_state *state = &sensor->state;
> > + struct device *dev = sensor->dev;
> > + int ret;
> > +
> > + switch (mask) {
> > + case IIO_CHAN_INFO_RAW:
> Need define scope {}
>
> > + unsigned int scan_index;
> > +
> > + ret = iio_device_claim_direct_mode(iio_dev);
> > + if (ret)
> > + return ret;
> > +
> > + ret = pm_runtime_resume_and_get(dev);
> > + if (ret < 0)
> > + goto release_direct_mode;
> > +
> > + if (sensor->scan_sync) {
>
> So this is curious. You just power up sensor and let it run until interrupt.
> I'd expect to have interrupt disabled unless buffered capture is in use.
> It would be fine to turn it on for a single cycle though if that makes capturing
> on demand simpler.
So that reflects the fact that getting synchronous readings from the sensor
does require reading the channels "soon after" the interrupt.
> > + scan_index = channel->scan_index;
> > +
> > + mutex_lock(&sensor->lock);
> > + *value_first = (int)sensor->scan.channels[scan_index];
> > + mutex_unlock(&sensor->lock);
> > + } else {
> > + ret = opt4048_data_read(sensor, channel->address,
> > + (u32 *)value_first);
> > + }
> > +
> > + pm_runtime_mark_last_busy(dev);
> > + pm_runtime_put_autosuspend(dev);
> > +
> > +release_direct_mode:
>
> I'd factor out the code above into a another function, then can avoid
> the ugly goto within a switch statement.
Okay.
> > + iio_device_release_direct_mode(iio_dev);
> > +
> > + if (ret < 0)
> > + return ret;
> > +
> > + return IIO_VAL_INT;
> > +
> > + case IIO_CHAN_INFO_SCALE:
> > + *value_first = 0;
> > + *value_second = OPT4048_SCALE_ULUX;
> > +
> > + return IIO_VAL_INT_PLUS_MICRO;
> > +
> > + case IIO_CHAN_INFO_INT_TIME:
> > + unsigned int index = 2 * state->conversion_time_index;
> > +
> > + *value_first = opt4048_conversion_time_available[index];
> > + index++;
> > + *value_second = opt4048_conversion_time_available[index];
> > +
> > + return IIO_VAL_INT_PLUS_MICRO;
> > +
> > + default:
> > + return -EINVAL;
> > + }
> > +}
>
> > +
> > +static int opt4048_iio_write_event_value(struct iio_dev *iio_dev,
> > + struct iio_chan_spec const *channel,
> > + enum iio_event_type type,
> > + enum iio_event_direction direction,
> > + enum iio_event_info info,
> > + int value_first, int value_second)
> > +{
> > + struct opt4048_sensor *sensor = iio_priv(iio_dev);
> > + struct opt4048_sensor_state *state = &sensor->state;
> > + u32 value;
> > + int ret;
> > +
> > + switch (direction) {
> > + case IIO_EV_DIR_RISING:
> > + value = (u32)value_first;
> > + opt4048_threshold_convert(value, state->threshold_high);
> > + break;
> > +
> > + case IIO_EV_DIR_FALLING:
> > + value = (u32)value_first;
> > + opt4048_threshold_convert(value, state->threshold_low);
> > + break;
> > +
> > + default:
> > + return -EINVAL;
> > + }
> > +
> > + if (pm_runtime_suspended(sensor->dev))
> > + return 0;
> I'm curious. Why isn't this an error return? Check for other cases of this.
> I think they should all return an error so userspace knows something unexpected
> is going on.
I'd expect userspace to be able to configure the threshold without anything
reading values at that moment. The point is to keep the sensor off.
> > +
> > + ret = opt4048_state_configure_threshold(sensor);
> > + if (ret)
> > + return ret;
>
> return opt...
>
> > +
> > + return 0;
> > +}
>
> > +
> > +static int opt4048_iio_write_event_config(struct iio_dev *iio_dev,
> > + struct iio_chan_spec const *channel,
> > + enum iio_event_type type,
> > + enum iio_event_direction direction,
> > + int active)
> Now takes a bool. Rebase on either my iio.git / testing branch or
> rc1 once available (probably later today).
Will do!
> > +{
> > + struct opt4048_sensor *sensor = iio_priv(iio_dev);
> > + struct opt4048_sensor_state *state = &sensor->state;
> > + int ret = 0;
> > +
> > + /* Threshold active are read in IRQ thread. */
> > + mutex_lock(&sensor->lock);
>
> guard()
>
> > +
> > + switch (direction) {
> > + case IIO_EV_DIR_RISING:
> > + state->threshold_high_active = !!active;
>
> This is why the signature became bool. All drivers had the same handling
> which made no sense :(
>
> FWIW you don't need the !! even before that change.
Yeah bool isn't really a thing in C but it just feels more consistent.
It would be necessary if I had defined threshold_high_active as a single-bit
field though.
>
>
> > + break;
> > +
> > + case IIO_EV_DIR_FALLING:
> > + state->threshold_low_active = !!active;
> > + break;
> > +
> > + default:
> > + ret = -EINVAL;
> When using guard. Can just return -EINVAL;
> > + goto complete;
> > + }
> > +
> > + if (pm_runtime_suspended(sensor->dev))
> > + goto complete;
> As above - direct return.
> Though fun question of what you should do if this does fail as
> we are then in a weird unknown state.
> why not an error return?
It's really not an error if it fails and the state handling is written to allow
configuring it with the device off. It just updates the states without reaching
the device and the state gets applied at the next resume.
> > +
> > + ret = opt4048_state_configure_threshold(sensor);
> Will become
> return opt4048_*
> > +
> > +complete:
> > + mutex_unlock(&sensor->lock);
> > +
> > + return ret;
> > +}
> };
> > +
> > +static irqreturn_t opt4048_iio_buffer_trigger(int irq, void *data)
> > +{
> > + struct iio_poll_func *poll_func = data;
> > + struct iio_dev *iio_dev = poll_func->indio_dev;
> > + struct opt4048_sensor *sensor = iio_priv(iio_dev);
> > + struct opt4048_sensor_scan scan = { 0 };
> > + s64 timestamp;
> > + unsigned int index = 0;
> > + unsigned int i;
> > + int ret;
> > +
> > + /* Capture timestamp just before reading values. */
> > + timestamp = iio_get_time_ns(iio_dev);
> > +
> > + mutex_lock(&sensor->lock);
> > +
> > + if (!sensor->scan_sync) {
> > + ret = opt4048_data_scan(sensor, &sensor->scan);
> So you have a weird hybrid of capture in the data ready interrupt and here.
The point is to allow both interrupt-based and interrupt-less operation here.
> Why not just kick this off by having a data ready trigger and
> using iio_trigger_poll() to effectively call this on the data ready
> interrupt or on an other trigger. That way should need no special handling
> for your sync scan.
See the comment above, maybe I missed something though.
>
>
> > + if (ret)
> > + goto complete;
> > + }
> > +
> > + for_each_set_bit(i, iio_dev->active_scan_mask, iio_dev->masklength) {
> > + /* Assume scan index matches array index. */
> > + const struct iio_chan_spec *channel = &opt4048_iio_channels[i];
> > + unsigned int scan_index = channel->scan_index;
> > +
> > + /* Only active channels are reported, in order. */
> > + scan.channels[index] = sensor->scan.channels[scan_index];
> > + index++;
>
> If you are always reading all channels, set avail_scan_masks and let the
> demux in the IIO core deal with this data rearranging.
Ah I didn't know we could do that!
> > + }
> > +
> > + iio_push_to_buffers_with_timestamp(iio_dev, &scan, timestamp);
> > +
> > +complete:
> > + mutex_unlock(&sensor->lock);
> > +
> > + iio_trigger_notify_done(iio_dev->trig);
> > +
> > + return IRQ_HANDLED;
> > +}
>
> ...
>
>
> > +static const struct iio_buffer_setup_ops opt4048_iio_buffer_setup_ops = {
> > + .preenable = opt4048_iio_buffer_preenable,
> > + .postdisable = opt4048_iio_buffer_postdisable,
> > +};
> > +
> > +static int opt4048_iio_setup(struct opt4048_sensor *sensor)
> This function doesn't add much. I'd just put the code inline in prbe.
I just like splitting framework-specific init, it makes things more readable.
> > +{
> > + struct iio_chan_spec *channels;
> > + struct iio_dev *iio_dev = sensor->iio_dev;
> Just pass it the iio_dev in.
My taste is rather to have a device-wide structure that is used in all
functions that are not callbacks from the framework/bus/device.
> > + struct device *dev = sensor->dev;
> > + unsigned int channels_count;
> > + int ret;
> > +
> > + channels_count = ARRAY_SIZE(opt4048_iio_channels);
> > +
> > + if (sensor->i2c_client->irq > 0) {
> > + channels = devm_kzalloc(dev, sizeof(opt4048_iio_channels),
> > + GFP_KERNEL);
>
> kmemdup. However I'd rather just see this picking between two static
> const arrays of channels. There are only two cases so it isn't worth dynamic
> channel setup complexity.
Well they would be 99 % identical. I don't really see how this would be a
beneficial duplication versus the low complexity of these few lines.
> > +
> > + memcpy(channels, opt4048_iio_channels,
> > + sizeof(opt4048_iio_channels));
> > +
> > + /* Attach threshold events to the illuminance channel. */
> > + channels[3].event_spec = opt4048_iio_events;
> > + channels[3].num_event_specs = ARRAY_SIZE(opt4048_iio_events);
> > + } else {
> > + /* Threshold events are not available without an irq. */
> > + channels = (struct iio_chan_spec *)opt4048_iio_channels;
> > + }
> > +
> > + iio_dev->info = &opt4048_iio_info;
> > + iio_dev->name = "opt4048";
> > + iio_dev->channels = channels;
> > + iio_dev->num_channels = channels_count;
> > + iio_dev->modes = INDIO_DIRECT_MODE;
> > +
> > + ret = devm_iio_triggered_buffer_setup(dev, iio_dev, NULL,
> > + opt4048_iio_buffer_trigger,
> > + &opt4048_iio_buffer_setup_ops);
> > + if (ret) {
> > + dev_err(dev, "failed to setup iio triggered buffer\n");
> > + return ret;
> > + }
> > +
> > + ret = iio_device_register(iio_dev);
>
> As below. No reason not to use devm_iio_device_register() and avoid
> need to manually unwind.
Oh sure!
> > + if (ret) {
> > + dev_err(dev, "failed to register iio device\n");
> > + return ret;
> > + }
> > +
> > + return 0;
> > +}
> > +
> > +static void opt4048_iio_cleanup(struct opt4048_sensor *sensor)
> > +{
> > + iio_device_unregister(sensor->iio_dev);
> This wrapper doesn't add anything useful so put the code inline.
> Though as below, it shouldn't be called directly at all.
>
>
> > +}
> > +
> > +/* IRQ */
> Comment doesn't add anything that isn't obvious from the code.
> So don't have it.
These are meant as categories to make it clear what the following code is
related to.
> > +
> > +static irqreturn_t opt4048_irq(int irq, void *data)
> > +{
> > + struct opt4048_sensor *sensor = data;
> > + struct opt4048_sensor_state *state = &sensor->state;
> > + struct iio_dev *iio_dev = sensor->iio_dev;
>
> Set data = iio_dev and then use iio_priv() on that.
> There shouldn't be a need to go back the other way.
Not really my taste, sorry.
> > + bool threshold_rising;
> > + bool threshold_falling;
> > + s64 timestamp;
> > + u64 code;
> Might as well combine elements of same type. Saves a few lines of code
> and doesn't harm readability much.
Sure.
> > + u16 status;
> > + int ret;
> > +
> > + timestamp = iio_get_time_ns(iio_dev);
> > +
> > + ret = i2c_smbus_read_word_swapped(sensor->i2c_client, OPT4048_STATUS);
> > + if (ret < 0)
> > + return IRQ_HANDLED;
> > +
> > + status = (u16)ret;
> > +
> > + mutex_lock(&sensor->lock);
> Use
> guard(mutex)(&sensor->lock);
> and include cleanup.h
>
> That will simplify this function as you can then do direct returns
> instead of gotos.
>
> > +
> > + if (!state->active)
> Perhaps add a comment for this. I guess it's about preventing a race?
Yes although I'm not even 100% sure it can really happen.
> + the lock prevents concurrent power down.
>
> I'd be tempted to just do a pm_runtime_resume_and_get()
> Vast majority of the time the device will be powered up and you won't
> need to do anything. The race will be closed as in the rare corner
> case w will be able ot read the data.
>
> I'm also curious that you can still read status after powering down.
> Maybe add a comment about that always being safe.
Mhh that's a good point. Status read should really fail indeed.
I guess I'll need to rework this.
> > + goto complete;
> > +
> > + if (status & OPT4048_STATUS_CONV_READY_FLAG)
> > + opt4048_data_scan(sensor, &sensor->scan);
>
> Superficially this looks like a data ready trigger should be used.
> So register a trigger and do the read in the trigger handler in all
> cases rather than here.
>
> > +
> > + threshold_falling = (status & OPT4048_STATUS_FLAG_L) &&
> > + !(state->status & OPT4048_STATUS_FLAG_L);
> > +
> > + if (state->threshold_low_active && threshold_falling) {
> > + code = IIO_UNMOD_EVENT_CODE(IIO_LIGHT, 3, IIO_EV_TYPE_THRESH,
> > + IIO_EV_DIR_FALLING);
> > + iio_push_event(iio_dev, code, timestamp);
> > + }
> > +
> > + threshold_rising = (status & OPT4048_STATUS_FLAG_H) &&
> > + !(state->status & OPT4048_STATUS_FLAG_H);
> > +
> > + if (state->threshold_high_active && threshold_rising) {
> > + code = IIO_UNMOD_EVENT_CODE(IIO_LIGHT, 3, IIO_EV_TYPE_THRESH,
> > + IIO_EV_DIR_RISING);
> > + iio_push_event(iio_dev, code, timestamp);
> > + }
> > +
> > + state->status = status;
> Add a comment on why you are saving this (I guess because the device
> interrupts every time otherwise). Does it interrupt on the reverse
> direction? If not how is this cleared?
Ah I thought it was clear from the fact that we use it in the statements above
to find out if we just crossed a threshold.
The device reports "value above/below threshold" at every reading and not
"threshold crossed" so I need to compare with previous state to derive
rising/falling.
> > +
> > +complete:
> > + mutex_unlock(&sensor->lock);
> > +
> > + return IRQ_HANDLED;
> > +}
> > +
> > +/* PM */
> Structural comments like this don't add much after a driver is
> written and often become wrong as a driver develops over time.
> I'd drop them.
>
> > +
> > +static int opt4048_suspend(struct device *dev)
> > +{
> > + struct opt4048_sensor *sensor = dev_get_drvdata(dev);
> > + int ret;
> > +
> > + ret = opt4048_power(sensor, 0);
> > + if (ret)
> > + goto error;
> > +
> > + ret = regulator_disable(sensor->vdd_supply);
> > + if (ret)
> > + goto error;
> > +
> > + return 0;
> > +
> > +error:
> > + return -EAGAIN;
> Direct returns preferred over both eating the error codes and
> a goto like this.
Hehe the suspend callback doesn't work that way. If you *ever* return something
else than -EAGAIN, it will never attempt to resume the device again. This is
documented in the runtime pm semantics.
So it is very crucial to eat the error code and return -EAGAIN if we want to
have a chance at it again. Many drivers get this wrong.
> > +}
> > +
> > +static int opt4048_resume(struct device *dev)
> > +{
> > + struct opt4048_sensor *sensor = dev_get_drvdata(dev);
> > + unsigned long sleep_min;
> > + unsigned int index;
> > + int ret;
> > +
> > + ret = regulator_enable(sensor->vdd_supply);
> > + if (ret)
> > + goto error;
> > +
> > + /* Wait for the regulator to settle and the chip to power-on. */
> > + udelay(30);
> > +
> > + ret = opt4048_state_configure(sensor);
> > + if (ret)
> > + goto error_regulator;
> > +
> > + ret = opt4048_power(sensor, 1);
> > + if (ret)
> > + goto error_regulator;
> > +
> > + /* Wait for conversion to be ready for all channels. */
> We might not have powered up for that reason but I guess this does
> little harm.
By design this driver only powers up the device for readings. Everything else
is kept staging in the state structure.
> > + index = 2 * sensor->state.conversion_time_index + 1;
> > + sleep_min = opt4048_conversion_time_available[index] * 4;
> > +
> > + usleep_range(sleep_min, 5 * sleep_min / 4);
>
> Perhaps use fsleep() to avoid anyone needing to reason about the margins
> etc.
>
> > +
> > + return 0;
> > +
> > +error_regulator:
> > + regulator_disable(sensor->vdd_supply);
> > +
> > +error:
> Direct returns make for easier to review code as following an error
> path doesn't require checking to see what the cleanup is.
I prefer it over duplication.
> > + return -EAGAIN;
> > +}
> > +
> > +static const struct dev_pm_ops opt4048_pm_ops = {
> > + .runtime_suspend = opt4048_suspend,
> > + .runtime_resume = opt4048_resume,
> > +};
> > +
> > +/* I2C */
> Not a particularly useful comment. I'd drop it.
> > +
> > +static int opt4048_probe(struct i2c_client *client)
> > +{
> > + struct device *dev = &client->dev;
> > + struct opt4048_sensor *sensor;
> > + struct iio_dev *iio_dev;
> > + int irq = client->irq;
> > + int ret;
> > +
> > + iio_dev = devm_iio_device_alloc(dev, sizeof(*sensor));
> > + if (!iio_dev)
> > + return -ENOMEM;
> > +
> > + sensor = iio_priv(iio_dev);
> > +
> > + sensor->dev = dev;
> > + sensor->i2c_client = client;
> > + sensor->iio_dev = iio_dev;
> That is almost always a sign that you have a less than ideal layering
> in the driver.
I disagree, my preference is to have a top-level device-specific structures
that holds everything else and is used in all local functions. It find it
a lot more convenient and readable.
> > +
> > + i2c_set_clientdata(client, sensor);
> > +
> > + mutex_init(&sensor->lock);
> ret = devm_mutex_init()
> if (ret)
> return ret;
> > +
> > + sensor->vdd_supply = devm_regulator_get(dev, "vdd");
>
> Given runtime PM may not even be enabled, you should turn the power on.
> Unconditionally.
The driver definitely depends on runtime PM being enabled (and I think it's
a good thing to keep the device off when it's not used).
> > + if (IS_ERR(sensor->vdd_supply)) {
> > + dev_err(dev, "failed to get VDD regulator\n");
>
> For error returns and messages in probe use return dev_err_probe()
> It will do various useful things for deferred and out of memory
> error prints. Find to use it in all code only called from the probe
> callback.
Ah right, I forgot about it.
> > + return PTR_ERR(sensor->vdd_supply);
> > + }
> > +
> > + opt4048_state_reset(sensor);
> > +
> > + devm_pm_runtime_enable(dev);
> Can fail so check for that.
>
> > +
> > + ret = opt4048_identify(sensor);
> > + if (ret)
> As above, failure to match should not be a reason to fail probe.
> > + return ret;
> > +
> > + pm_runtime_set_autosuspend_delay(dev, 5000);
> > + pm_runtime_use_autosuspend(dev);
> > +
> > + if (irq > 0) {
> > + ret = devm_request_threaded_irq(dev, irq, NULL, opt4048_irq,
> > + IRQF_TRIGGER_FALLING |
> > + IRQF_ONESHOT,
> Direction should come from firmware (so don't specify it here).
> It's reasonably common to find an inverter in the path for an interrupt
> and so we can't know the sense in the driver.
Understood.
> > + "opt4048", sensor);
> > + if (ret) {
> > + dev_err(dev, "failed to request irq\n");
> > + return ret;
> > + }
> > +
> > + sensor->scan_sync = true;
> > + }
> > +
> > + ret = opt4048_iio_setup(sensor);
> As above, I'd put the contents of that function here as well. Whilst
> I guess you wanted to keep functions small, sometimes that just leads
> to less readable code than a long function with everything we expect
> in it.
>
> > + if (ret)
> > + return ret;
> > +
> > + return 0;
>
> > +}
> > +
> > +static void opt4048_remove(struct i2c_client *client)
> > +{
> > + struct opt4048_sensor *sensor = i2c_get_clientdata(client);
> > +
> > + opt4048_iio_cleanup(sensor);
> For a simple single call like this, use a devm_add_action_or_reset()
> so that the cleanup is associated directly with the setup function and
> we don't need a remove() callback at all.
>
> Given all it does is iio_device_unregister() just use devm_device_register()
> in the first place and drop this.
Alright.
> > +}
> > +
> > +static const struct i2c_device_id opt4048_id[] = {
> > + { "opt4048" },
> > + { }
> > +};
> > +
> > +MODULE_DEVICE_TABLE(i2c, opt4048_id);
> > +
> > +static const struct of_device_id opt4048_of_match[] = {
> > + { .compatible = "ti,opt4048" },
> > + { }
> > +};
> Similar to below, fairly standard to group the id table and
> MODULE_DEVICE_TABLE but not having a blank line.
I don't particularly like it.
> > +
> > +MODULE_DEVICE_TABLE(of, opt4048_of_match);
> > +
> > +static struct i2c_driver opt4048_driver = {
> > + .driver = {
> > + .name = "opt4048",
> > + .of_match_table = opt4048_of_match,
> > + .pm = &opt4048_pm_ops,
> > + },
> > + .id_table = opt4048_id,
> > + .probe = opt4048_probe,
> > + .remove = opt4048_remove,
> > +};
> > +
> It's common to not put a blank line here given how closely coupled this
> macro is to the structure.
> > +module_i2c_driver(opt4048_driver);
> > +
> > +MODULE_AUTHOR("Paul Kocialkowski <paulk@sys-base.io>");
> > +MODULE_DESCRIPTION("Texas Instruments OPT4048 XYZ tristimulus color sensor driver");
> > +MODULE_LICENSE("GPL");
>
--
Paul Kocialkowski,
Independent contractor - sys-base - https://www.sys-base.io/
Free software developer - https://www.paulk.fr/
Expert in multimedia, graphics and embedded hardware support with Linux.
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 1/2] dt-bindings: iio: Add TI OPT4048 color sensor bindings
2024-11-30 17:42 [PATCH 1/2] dt-bindings: iio: Add TI OPT4048 color sensor bindings Paul Kocialkowski
2024-11-30 17:42 ` [PATCH 2/2] iio: light: Add support for the TI OPT4048 color sensor Paul Kocialkowski
@ 2024-12-02 8:41 ` Krzysztof Kozlowski
1 sibling, 0 replies; 10+ messages in thread
From: Krzysztof Kozlowski @ 2024-12-02 8:41 UTC (permalink / raw)
To: Paul Kocialkowski
Cc: linux-iio, devicetree, linux-kernel, Jonathan Cameron,
Lars-Peter Clausen, Rob Herring, Krzysztof Kozlowski,
Conor Dooley
On Sat, Nov 30, 2024 at 06:42:11PM +0100, Paul Kocialkowski wrote:
> The Texas Instruments OPT4048 is a XYZ tristimulus color sensor.
>
> It requires a VDD power supply and can optionally support an interrupt.
A nit, subject: drop second/last, redundant "bindings". The
"dt-bindings" prefix is already stating that these are bindings.
See also:
https://elixir.bootlin.com/linux/v6.7-rc8/source/Documentation/devicetree/bindings/submitting-patches.rst#L18
>
> Signed-off-by: Paul Kocialkowski <paulk@sys-base.io>
> ---
> .../bindings/iio/light/ti,opt4048.yaml | 54 +++++++++++++++++++
> 1 file changed, 54 insertions(+)
> create mode 100644 Documentation/devicetree/bindings/iio/light/ti,opt4048.yaml
>
> diff --git a/Documentation/devicetree/bindings/iio/light/ti,opt4048.yaml b/Documentation/devicetree/bindings/iio/light/ti,opt4048.yaml
> new file mode 100644
> index 000000000000..e2b7472ab588
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/iio/light/ti,opt4048.yaml
> @@ -0,0 +1,54 @@
> +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/iio/light/ti,opt4048.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Texas Instruments OPT4048 XYZ tristimulus color sensor
> +
> +maintainers:
> + - Paul Kocialkowski <paulk@sys-base.io>
> +
> +description: |
Do not need '|' unless you need to preserve formatting.
> + The device supports both interrupt-driven and interrupt-less operation,
> + depending on whether an interrupt property is present in the device-tree.
> +
> +properties:
> + compatible:
> + const: ti,opt4048
> +
> + reg:
> + maxItems: 1
> +
> + interrupts:
> + maxItems: 1
> + description: |
Do not need '|' unless you need to preserve formatting.
> + The interrupt is detected on a falling edge, with a low level asserted
> + for 1 us. It might be missed because of hardware interrupt debouncing
> + mechanisms due to this short time.
> +
> + vdd-supply: true
> +
> +additionalProperties: false
This goes after required: block.
> +
> +required:
> + - compatible
> + - reg
> + - vdd-supply
> +
> +examples:
> + - |
> + #include <dt-bindings/interrupt-controller/irq.h>
> + i2c {
> + #address-cells = <1>;
> + #size-cells = <0>;
> +
> + light-sensor@44 {
> + compatible = "ti,opt4048";
Messed indentation, keep 4 spaces here.
With all above changes:
Reviewed-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
Best regards,
Krzysztof
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 2/2] iio: light: Add support for the TI OPT4048 color sensor
2024-12-01 17:56 ` Paul Kocialkowski
@ 2024-12-02 11:06 ` Jonathan Cameron
2024-12-02 14:55 ` Mehdi Djait
2024-12-04 14:48 ` Paul Kocialkowski
0 siblings, 2 replies; 10+ messages in thread
From: Jonathan Cameron @ 2024-12-02 11:06 UTC (permalink / raw)
To: Paul Kocialkowski
Cc: Jonathan Cameron, linux-iio, devicetree, linux-kernel,
Lars-Peter Clausen, Rob Herring, Krzysztof Kozlowski,
Conor Dooley
On Sun, 1 Dec 2024 18:56:30 +0100
Paul Kocialkowski <paulk@sys-base.io> wrote:
> Hi Jonathan,
>
> Le Sun 01 Dec 24, 11:55, Jonathan Cameron a écrit :
> > On Sat, 30 Nov 2024 18:42:12 +0100
> > Paul Kocialkowski <paulk@sys-base.io> wrote:
> >
> > > The Texas Instruments OPT4048 is a XYZ tristimulus color sensor,
> > > with an additional wide (visible + IR) channel.
> > >
> > > This driver implements support for all channels, with configurable
> > > integration time and auto-gain. Both direct reading and
> > > triggered-buffer modes are supported.
> > >
> > > Note that the Y channel is also reported as a separate illuminance
> > > channel, for which a scale is provided (following the datasheet) to
> > > convert it to lux units. Falling and rising thresholds are supported
> > > for this channel.
> > >
> > > The device's interrupt can be used to sample all channels at the end
> > > of conversion and is optional.
> > >
> > > Signed-off-by: Paul Kocialkowski <paulk@sys-base.io>
> > Hi Paul,
> >
> > Various comments inline. Most significant is that this seems to be
> > suitable for a simple dataready trigger that will make your various
> > interrupt and non interrupt flows more similar.
>
> And thanks for the fast review and insightful comments!
>
> I considered implementing a trigger in the driver, but the issue I found
> is that the trigger is expected to be called from hard irq context,
> while the new values are read in the bottom half.
The trigger can be called from either the hard irq context or from
a thread. See iio_trigger_poll_nested()
There is a quirk that you then don't end up calling the registered
hard irq handler for the trigger so sometimes a bit of fiddly code
is needed to ensure timestamps etc are grabbed. Not sure that matters
here.
> I understand the triggered
> buffer callbacks are executed as a thread as well, so there would be race
> between the two which could result in previous values being returned.
With the above nested call it is all run in the same thread
See handle_nested_irq() in particular the function docs.
https://elixir.bootlin.com/linux/v6.12.1/source/kernel/irq/chip.c#L459
> So I concluded that it was more beneficial to preserve the synchronous reading
> mechanism over implementing the trigger.
Definite preference for a trigger approach, but I may well still be missing
a detail.
Jonathan
>
> But maybe I missed/misunderstood something here.
>
> > Jonathan
> >
> > > diff --git a/drivers/iio/light/Makefile b/drivers/iio/light/Makefile
> > > index 321010fc0b93..f2031e6236f9 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_OPT4048) += opt4048.o
> > > obj-$(CONFIG_PA12203001) += pa12203001.o
> > > obj-$(CONFIG_ROHM_BU27008) += rohm-bu27008.o
> > > obj-$(CONFIG_ROHM_BU27034) += rohm-bu27034.o
> > > diff --git a/drivers/iio/light/opt4048.c b/drivers/iio/light/opt4048.c
> > > new file mode 100644
> > > index 000000000000..1ad5e6586aad
> > > --- /dev/null
> > > +++ b/drivers/iio/light/opt4048.c
> > > @@ -0,0 +1,1145 @@
> > > +// SPDX-License-Identifier: GPL-2.0-only
> > > +/*
> > > + * Copyright 2024 Paul Kocialkowski <paulk@sys-base.io>
> > > + */
> > > +
> > > +#include <linux/delay.h>
> > > +#include <linux/i2c.h>
> > > +#include <linux/iio/events.h>
> > > +#include <linux/iio/iio.h>
> > > +#include <linux/iio/trigger_consumer.h>
> > > +#include <linux/iio/triggered_buffer.h>
> > > +#include <linux/log2.h>
> > > +#include <linux/module.h>
> > > +#include <linux/pm_runtime.h>
> > > +#include <linux/regulator/consumer.h>
> > > +
> > > +#define OPT4048_CH0_DATA0 0x0
> > > +#define OPT4048_CH0_DATA1 0x1
> > > +#define OPT4048_CH1_DATA0 0x2
> > > +#define OPT4048_CH1_DATA1 0x3
> > > +#define OPT4048_CH2_DATA0 0x4
> > > +#define OPT4048_CH2_DATA1 0x5
> > > +#define OPT4048_CH3_DATA0 0x6
> > > +#define OPT4048_CH3_DATA1 0x7
> > > +
> > > +#define OPT4048_CH_DATA0_MSB_VALUE(v) ((v) & GENMASK(11, 0))
> > > +#define OPT4048_CH_DATA0_EXPONENT_VALUE(v) (((v) & GENMASK(15, 12)) >> 12)
> >
> > For all these - just define the masks and use FIELD_GET() / FIELD_PREP()
> > inline. Masks should be defined with names that make it clear what they are
> > masking and in what registers.
>
> I see. Note that I generally tried to follow the datasheet's terminology for
> convenience.
Understood, but you can stay near to that with well named masks etc
and generally it remains pretty readable and compact.
>
> > > +};
> > > +
> > > +struct opt4048_sensor {
> > > + struct device *dev;
> > > + struct i2c_client *i2c_client;
> > > + struct iio_dev *iio_dev;
> > > +
> > > + struct regulator *vdd_supply;
> > > +
> > > + struct opt4048_sensor_state state;
> >
> > > + struct opt4048_sensor_scan scan;
> >
> > As below. I think you should just always do the read in the trigger handler.
>
> As far as I could see the hardware doesn't guarantee that values come from
> the same read sequence (even if read as a single i2c transaction). This is why
> my approach was to read them as early as possible after the irq is signaled
> and cache them here, instead of doing the actual read in the trigger handler
> or sysfs read callback (which may have both values from the previous and next
> reads).
As above, if you do it via handle_nested_irq, it ends up being very nearly the
same time as you have it here (a few calls later).
>
> The only thing the hardware guarantees is that the DATA1 value is latched when
> DATA0 is read.
Ah good. So no tearing at least.
Does it actually matter if you get reads from neighbouring samples? Light sensors
tend not to be measuring anything particularly dynamic. I doubt you'll have problems
anyway, but if you do I'm not sure I see it as mattering.
>
> > > + bool scan_sync;
> >
> > This needs a comment. Interrupts and sync aren't something that normally goes
> > together so the name is somewhat confusing.
>
> Okay. The idea is to have a synchronous read where all channels are read as
> soon as conversion done is signaled.
That's not synchronous though, as they are done on an async interrupt.
So maybe thing of a different term.
> > > +
> > > + /* Report illuminance using Y intensity value. */
> >
> > That seems 'interesting'. Why?
>
> The Y channel is defined by CIE1931 to match the (photopic) illuminance response
> curve. The main difference is that Y is unit-less while illuminance is defined
> in lux. This specific channel gets an extra scale property to report the
> conversion coefficient for lux.
Ah I had it in my head that the curves were similar but not the same.
Fair enough.
>
> However we still need the Y value to be reported as such in order to be related
> to X and Z.
>
> > > + scan->channels[4] = scan->channels[1];
> > > +
> > > + return 0;
> > > +}
> > > +
> > > +static int opt4048_identify(struct opt4048_sensor *sensor)
> > > +{
> > > + struct device *dev = sensor->dev;
> > > + int ret;
> > > + u16 id, low, high;
> > > +
> > > + ret = pm_runtime_resume_and_get(dev);
> > > + if (ret < 0)
> > > + return ret;
> > > +
> > > + ret = i2c_smbus_read_word_swapped(sensor->i2c_client, OPT4048_DID);
> > > + if (ret < 0)
> > > + goto complete;
> > > +
> > > + low = (u16)OPT4048_DID_L_VALUE(ret);
> > > + high = (u16)OPT4048_DID_H_VALUE(ret);
> > > +
> > > + id = OPT4048_DID_VALUE(low, high);
> > > +
> > > + switch (id) {
> > > + case OPT4048_DID_OPT4048:
> > > + dev_info(dev, "identified OPT4048 sensor\n");
> > This isn't useful information and can be easily established once the
> > driver has loaded. So dev_dbg() at most.
>
> Honestly I see a bunch of driver doing that (especially camera sensors)
> and find it generally useful. But I don't mind getting rid of it.
I try to squash them. Spend too long staring at enormously long kernel
boot logs :) I'm fine if it provides extra info though. Some devices have
serial numbers or firmware version numbers etc.
>
> > > + ret = 0;
> > > + break;
> > > + default:
> > > + dev_err(dev, "unknown sensor with id: %#x\n", id);
> > > + ret = -ENODEV;
> > We shouldn't treat a failure to match the ID as a reason to fail probe.
> > Consider the use of fallback IDs in device tree with older kernels.
> > If a new device comes along that is backwards compatible, we want that
> > to work with out driver modifications.
>
> Honestly this is really just a way to check that i2c transaction are working,
> that the device is actually there and to report when it's not.
>
> I'm not sure it's such a good idea to assume that other IDs would not be
> a sign that something very wrong is going on (typically some other device
> connected on the same i2c address). Failing to probe will prevent
> misconfiguring another device.
>
> Just adding another case here if a new device is ever introduced doesn't
> sound like such a big drawback in comparison.
I used to agree with you, but view of DT maintainers if that if you have a broken
board or device or a wrong firmware then it isn't the bindings or Linux's problem
to solve. Given we have a 'lot' of fallback compatibles now I'm now pretty
convinced on this.
An info message should be enough to show something is odd and give the breadcrumbs
needed to debug.
>
> > It's fine to print an dev_info message though so the user is aware
> > that we are ignoring the missmatch.
> >
> > > + break;
> > > + }
> > > +
> > > +complete:
> > > + pm_runtime_put_sync(dev);
> > > +
> > > + return ret;
> > > +}
> > > +
> > > +static int opt4048_power(struct opt4048_sensor *sensor, bool on)
> > > +{
> > > + struct opt4048_sensor_state *state = &sensor->state;
> > > + int ret;
> > > +
> > > + mutex_lock(&sensor->lock);
> > guard()
> > > +
> > > + state->active = on;
> > > + ret = opt4048_state_configure_cfg0(sensor);
> > > +
> > > + mutex_unlock(&sensor->lock);
> > > +
> > > + return ret;
> > > +}
> >
> > > +static int opt4048_state_configure_cfg1(struct opt4048_sensor *sensor)
> > > +{
> > > + u16 value;
> > > + int ret;
> > I don't really see any advantage in these wrappers. I'd just have
> > the code inline in opt4048_state_configure()
>
> Well I do think they are important to make the code more digest.
As someone who digested it. They don't make it easier. They just
make me scroll backwards and forwards a lot whilst reviewing.
>
> > > +
> > > + /* Assign threshold to the Y channel for illuminance. */
> > > + value = OPT4048_CFG1_I2C_BURST |
> > > + OPT4048_CFG1_INT_DIR_OUT |
> > > + OPT4048_CFG1_THRESHOLD_CH_SEL(1) |
> > > + OPT4048_CFG1_RESERVED;
> >
> > combine these on fewer lines. Just generally stay below 80 chars unless
> > there is a strong readability reason to go longer.
>
> I find it a lot more readable with one item per line.
Hmm. In this particular case I don't mind that much.
It is common for these to be come stupidly long for no real readability
advantage.
>
> > > +
> > > + if (sensor->scan_sync)
> > > + value |= OPT4048_CFG1_INT_CFG_DATA_READY_ALL;
> > > + else
> > > + value |= OPT4048_CFG1_INT_CFG_ALERT;
> > > +
> > > + ret = i2c_smbus_write_word_swapped(sensor->i2c_client, OPT4048_CFG1,
> > > + value);
> > > + if (ret < 0)
> > > + return ret;
> > > +
> > > + return 0;
> > > +}
> > > +static int opt4048_iio_read_raw(struct iio_dev *iio_dev,
> > > + struct iio_chan_spec const *channel,
> > > + int *value_first, int *value_second, long mask)
> > > +{
> > > + struct opt4048_sensor *sensor = iio_priv(iio_dev);
> > > + struct opt4048_sensor_state *state = &sensor->state;
> > > + struct device *dev = sensor->dev;
> > > + int ret;
> > > +
> > > + switch (mask) {
> > > + case IIO_CHAN_INFO_RAW:
> > Need define scope {}
> >
> > > + unsigned int scan_index;
> > > +
> > > + ret = iio_device_claim_direct_mode(iio_dev);
> > > + if (ret)
> > > + return ret;
> > > +
> > > + ret = pm_runtime_resume_and_get(dev);
> > > + if (ret < 0)
> > > + goto release_direct_mode;
> > > +
> > > + if (sensor->scan_sync) {
> >
> > So this is curious. You just power up sensor and let it run until interrupt.
> > I'd expect to have interrupt disabled unless buffered capture is in use.
> > It would be fine to turn it on for a single cycle though if that makes capturing
> > on demand simpler.
>
> So that reflects the fact that getting synchronous readings from the sensor
> does require reading the channels "soon after" the interrupt.
For the direct read it is fine to grab them in the interrupt. Just run
it with direct mode claimed an a check in your interrupt handler on whether
the buffer is enabled. iio_device_claim_direct_mode() will prevent any
transitions happening into buffered mode concurrent with this single shot
read.
>
> > > +static int opt4048_iio_write_event_value(struct iio_dev *iio_dev,
> > > + struct iio_chan_spec const *channel,
> > > + enum iio_event_type type,
> > > + enum iio_event_direction direction,
> > > + enum iio_event_info info,
> > > + int value_first, int value_second)
> > > +{
> > > + struct opt4048_sensor *sensor = iio_priv(iio_dev);
> > > + struct opt4048_sensor_state *state = &sensor->state;
> > > + u32 value;
> > > + int ret;
> > > +
> > > + switch (direction) {
> > > + case IIO_EV_DIR_RISING:
> > > + value = (u32)value_first;
> > > + opt4048_threshold_convert(value, state->threshold_high);
> > > + break;
> > > +
> > > + case IIO_EV_DIR_FALLING:
> > > + value = (u32)value_first;
> > > + opt4048_threshold_convert(value, state->threshold_low);
> > > + break;
> > > +
> > > + default:
> > > + return -EINVAL;
> > > + }
> > > +
> > > + if (pm_runtime_suspended(sensor->dev))
> > > + return 0;
> > I'm curious. Why isn't this an error return? Check for other cases of this.
> > I think they should all return an error so userspace knows something unexpected
> > is going on.
>
> I'd expect userspace to be able to configure the threshold without anything
> reading values at that moment. The point is to keep the sensor off.
Ah. I misread this. How is the race prevented with it being enabled
at this point?
>
> > > +
> > > + ret = opt4048_state_configure_threshold(sensor);
> > > + if (ret)
> > > + return ret;
> >
> > return opt...
> >
> > > +
> > > + return 0;
> > > +}
> > > +
> > > + switch (direction) {
> > > + case IIO_EV_DIR_RISING:
> > > + state->threshold_high_active = !!active;
> >
> > This is why the signature became bool. All drivers had the same handling
> > which made no sense :(
> >
> > FWIW you don't need the !! even before that change.
>
> Yeah bool isn't really a thing in C but it just feels more consistent.
> It would be necessary if I had defined threshold_high_active as a single-bit
> field though.
Don't do that. Looses the clear meaning as a true / false field.
In kernel c bool is well defined (and in more recent C specs).
>
> >
> >
> > > + break;
> > > +
> > > + case IIO_EV_DIR_FALLING:
> > > + state->threshold_low_active = !!active;
> > > + break;
> > > +
> > > + default:
> > > + ret = -EINVAL;
> > When using guard. Can just return -EINVAL;
> > > + goto complete;
> > > + }
> > > +
> > > + if (pm_runtime_suspended(sensor->dev))
> > > + goto complete;
> > As above - direct return.
> > Though fun question of what you should do if this does fail as
> > we are then in a weird unknown state.
> > why not an error return?
>
> It's really not an error if it fails and the state handling is written to allow
> configuring it with the device off. It just updates the states without reaching
> the device and the state gets applied at the next resume.
I'd misread the code. Feels like there is a race here however.
>
> > > +
> > > + ret = opt4048_state_configure_threshold(sensor);
> > Will become
> > return opt4048_*
> > > +
> > > +complete:
> > > + mutex_unlock(&sensor->lock);
> > > +
> > > + return ret;
> > > +}
> > };
> > > +
> > > +static irqreturn_t opt4048_iio_buffer_trigger(int irq, void *data)
> > > +{
> > > + struct iio_poll_func *poll_func = data;
> > > + struct iio_dev *iio_dev = poll_func->indio_dev;
> > > + struct opt4048_sensor *sensor = iio_priv(iio_dev);
> > > + struct opt4048_sensor_scan scan = { 0 };
> > > + s64 timestamp;
> > > + unsigned int index = 0;
> > > + unsigned int i;
> > > + int ret;
> > > +
> > > + /* Capture timestamp just before reading values. */
> > > + timestamp = iio_get_time_ns(iio_dev);
> > > +
> > > + mutex_lock(&sensor->lock);
> > > +
> > > + if (!sensor->scan_sync) {
> > > + ret = opt4048_data_scan(sensor, &sensor->scan);
> > So you have a weird hybrid of capture in the data ready interrupt and here.
>
> The point is to allow both interrupt-based and interrupt-less operation here.
>
> > Why not just kick this off by having a data ready trigger and
> > using iio_trigger_poll() to effectively call this on the data ready
> > interrupt or on an other trigger. That way should need no special handling
> > for your sync scan.
>
> See the comment above, maybe I missed something though.
I think you do need a path for an early read, but that doesn't stop you
doing simple triggered capture in the trigger handler (allowing multiple types
of trigger). Just need to fix the mode so we don't enter buffered mode (see above
stuff on claiming direct mode) then check for whether the buffer is enabled
here. Grab a copy it if it isn't.
>
> >
> >
> > > + if (ret)
> > > + goto complete;
> > > + }
> > > +
> > > +static const struct iio_buffer_setup_ops opt4048_iio_buffer_setup_ops = {
> > > + .preenable = opt4048_iio_buffer_preenable,
> > > + .postdisable = opt4048_iio_buffer_postdisable,
> > > +};
> > > +
> > > +static int opt4048_iio_setup(struct opt4048_sensor *sensor)
> > This function doesn't add much. I'd just put the code inline in prbe.
>
> I just like splitting framework-specific init, it makes things more readable.
I'm not convinced. I wouldn't mind you doing some sub steps such
as buffer setup, trigger setup etc.
Maybe it's because it is the ordering in that code that goes wrong most
often + I'd rather all the stuff was grouped by functionality, not by
subsystem. So register trigger than get interrupt etc. Similar with buffer.
Then finally see that the exposure of sysfs controls is done in
probe()
>
> > > +{
> > > + struct iio_chan_spec *channels;
> > > + struct iio_dev *iio_dev = sensor->iio_dev;
> > Just pass it the iio_dev in.
>
> My taste is rather to have a device-wide structure that is used in all
> functions that are not callbacks from the framework/bus/device.
We have had a lot of broken code via sensor->iio_dev type
code in the past. I'm not going down that route again so
no to having a sensor->iio_dev.
Structures should have clean nesting. It can be done either way
around - but it's a case of pick one and stick to it.
1. Subsystem structure inside the device specific one.
2. Device specific one inside the subsystem one.
IIO does 2. So I don't want to see mixing and matching the two
models.
>
> > > + struct device *dev = sensor->dev;
> > > + unsigned int channels_count;
> > > + int ret;
> > > +
> > > + channels_count = ARRAY_SIZE(opt4048_iio_channels);
> > > +
> > > + if (sensor->i2c_client->irq > 0) {
> > > + channels = devm_kzalloc(dev, sizeof(opt4048_iio_channels),
> > > + GFP_KERNEL);
> >
> > kmemdup. However I'd rather just see this picking between two static
> > const arrays of channels. There are only two cases so it isn't worth dynamic
> > channel setup complexity.
>
> Well they would be 99 % identical. I don't really see how this would be a
> beneficial duplication versus the low complexity of these few lines.
Generally static const data ends up cleaner in the long run, but sure
we can refactor when it becomes more complex.
A lot of drivers become very very complex in this regard and we end
up ripping out code like this a lot in favor of const data.
> > Comment doesn't add anything that isn't obvious from the code.
> > So don't have it.
>
> These are meant as categories to make it clear what the following code is
> related to.
Sure. They seem like a good idea when people write new drivers and then we
have to go repair them when they break. They do that far too often.
Hence, preference to not have them in the first place.
Don't need a comment about irq to tell a function that is well named
like this one is about irqs!
>
> > > +
> > > +static irqreturn_t opt4048_irq(int irq, void *data)
> > > +{
> > > + struct opt4048_sensor *sensor = data;
> > > + struct opt4048_sensor_state *state = &sensor->state;
> > > + struct iio_dev *iio_dev = sensor->iio_dev;
> >
> > Set data = iio_dev and then use iio_priv() on that.
> > There shouldn't be a need to go back the other way.
>
> Not really my taste, sorry.
See above. Very strong preference for not mixing and matching
styles.
> > > +
> > > + state->status = status;
> > Add a comment on why you are saving this (I guess because the device
> > interrupts every time otherwise). Does it interrupt on the reverse
> > direction? If not how is this cleared?
>
> Ah I thought it was clear from the fact that we use it in the statements above
> to find out if we just crossed a threshold.
>
> The device reports "value above/below threshold" at every reading and not
> "threshold crossed" so I need to compare with previous state to derive
> rising/falling.
Ok. So it interrupts every time out of the window. That's horrible.
Maybe add a comment on that. I was kind of assuming it didn't interrupt again,
just that the status bits were set.
> > > +
> > > +static int opt4048_suspend(struct device *dev)
> > > +{
> > > + struct opt4048_sensor *sensor = dev_get_drvdata(dev);
> > > + int ret;
> > > +
> > > + ret = opt4048_power(sensor, 0);
> > > + if (ret)
> > > + goto error;
> > > +
> > > + ret = regulator_disable(sensor->vdd_supply);
> > > + if (ret)
> > > + goto error;
> > > +
> > > + return 0;
> > > +
> > > +error:
> > > + return -EAGAIN;
> > Direct returns preferred over both eating the error codes and
> > a goto like this.
>
> Hehe the suspend callback doesn't work that way. If you *ever* return something
> else than -EAGAIN, it will never attempt to resume the device again. This is
> documented in the runtime pm semantics.
>
> So it is very crucial to eat the error code and return -EAGAIN if we want to
> have a chance at it again. Many drivers get this wrong.
There is an argument that if you fail in any of the above calls the state
is completely unknown and the right thing to do is never resume. It's
a case of cross your fingers and hope.
>
> > > +}
> > > +
> > > +static int opt4048_resume(struct device *dev)
> > > +{
> > > + struct opt4048_sensor *sensor = dev_get_drvdata(dev);
> > > + unsigned long sleep_min;
> > > + unsigned int index;
> > > + int ret;
> > > +
> > > + ret = regulator_enable(sensor->vdd_supply);
> > > + if (ret)
> > > + goto error;
> > > +
> > > + /* Wait for the regulator to settle and the chip to power-on. */
> > > + udelay(30);
> > > +
> > > + ret = opt4048_state_configure(sensor);
> > > + if (ret)
> > > + goto error_regulator;
> > > +
> > > + ret = opt4048_power(sensor, 1);
> > > + if (ret)
> > > + goto error_regulator;
> > > +
> > > + /* Wait for conversion to be ready for all channels. */
> > We might not have powered up for that reason but I guess this does
> > little harm.
>
> By design this driver only powers up the device for readings. Everything else
> is kept staging in the state structure.
I'd misread some of the earlier code. Thanks for the clarification.
>
> > > + index = 2 * sensor->state.conversion_time_index + 1;
> > > + sleep_min = opt4048_conversion_time_available[index] * 4;
> > > +
> > > + usleep_range(sleep_min, 5 * sleep_min / 4);
> >
> > Perhaps use fsleep() to avoid anyone needing to reason about the margins
> > etc.
> >
> > > +
> > > + return 0;
> > > +
> > > +error_regulator:
> > > + regulator_disable(sensor->vdd_supply);
> > > +
> > > +error:
> > Direct returns make for easier to review code as following an error
> > path doesn't require checking to see what the cleanup is.
>
> I prefer it over duplication.
Hmm. We disagree on this. What I see is code where I have to go
check the error handler on paths where it is completely pointless as
it's not doing anything and I could have just seen the return at
the goto.
>
> > > + return -EAGAIN;
> > > +}
> > > +
> > > +static const struct dev_pm_ops opt4048_pm_ops = {
> > > + .runtime_suspend = opt4048_suspend,
> > > + .runtime_resume = opt4048_resume,
> > > +};
> > > +
> > > +/* I2C */
> > Not a particularly useful comment. I'd drop it.
> > > +
> > > +static int opt4048_probe(struct i2c_client *client)
> > > +{
> > > + struct device *dev = &client->dev;
> > > + struct opt4048_sensor *sensor;
> > > + struct iio_dev *iio_dev;
> > > + int irq = client->irq;
> > > + int ret;
> > > +
> > > + iio_dev = devm_iio_device_alloc(dev, sizeof(*sensor));
> > > + if (!iio_dev)
> > > + return -ENOMEM;
> > > +
> > > + sensor = iio_priv(iio_dev);
> > > +
> > > + sensor->dev = dev;
> > > + sensor->i2c_client = client;
> > > + sensor->iio_dev = iio_dev;
> > That is almost always a sign that you have a less than ideal layering
> > in the driver.
>
> I disagree, my preference is to have a top-level device-specific structures
> that holds everything else and is used in all local functions. It find it
> a lot more convenient and readable.
See above. Whilst we are going to disagree on this, I'm going to hold firm
because I do not want other driver authors copying this pattern. It makes
my job as a reviewer harder because there are two ways to do things when
there should only be one.
>
> > > +
> > > + i2c_set_clientdata(client, sensor);
> > > +
> > > + mutex_init(&sensor->lock);
> > ret = devm_mutex_init()
> > if (ret)
> > return ret;
> > > +
> > > + sensor->vdd_supply = devm_regulator_get(dev, "vdd");
> >
> > Given runtime PM may not even be enabled, you should turn the power on.
> > Unconditionally.
>
> The driver definitely depends on runtime PM being enabled (and I think it's
> a good thing to keep the device off when it's not used).
Generally a driver should depend on runtime_pm. What in here makes
that necessary (rather than a good idea for power saving!)?
If it does depend on it, then require it via kconfig depends.
But be prepared for push back on that as it shouldn't be necessary.
Jonathan
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 2/2] iio: light: Add support for the TI OPT4048 color sensor
2024-12-02 11:06 ` Jonathan Cameron
@ 2024-12-02 14:55 ` Mehdi Djait
2024-12-04 14:49 ` Paul Kocialkowski
2024-12-04 14:48 ` Paul Kocialkowski
1 sibling, 1 reply; 10+ messages in thread
From: Mehdi Djait @ 2024-12-02 14:55 UTC (permalink / raw)
To: Jonathan Cameron
Cc: Paul Kocialkowski, Jonathan Cameron, linux-iio, devicetree,
linux-kernel, Lars-Peter Clausen, Rob Herring,
Krzysztof Kozlowski, Conor Dooley
Hello Paul :)
On Mon, Dec 02, 2024 at 11:06:59AM +0000, Jonathan Cameron wrote:
> On Sun, 1 Dec 2024 18:56:30 +0100
> Paul Kocialkowski <paulk@sys-base.io> wrote:
>
> > Hi Jonathan,
> >
> > Le Sun 01 Dec 24, 11:55, Jonathan Cameron a écrit :
> > > On Sat, 30 Nov 2024 18:42:12 +0100
> > > Paul Kocialkowski <paulk@sys-base.io> wrote:
> > >
> > > > The Texas Instruments OPT4048 is a XYZ tristimulus color sensor,
> > > > with an additional wide (visible + IR) channel.
> > > >
> > > > This driver implements support for all channels, with configurable
> > > > integration time and auto-gain. Both direct reading and
> > > > triggered-buffer modes are supported.
> > > >
> > > > Note that the Y channel is also reported as a separate illuminance
> > > > channel, for which a scale is provided (following the datasheet) to
> > > > convert it to lux units. Falling and rising thresholds are supported
> > > > for this channel.
> > > >
> > > > The device's interrupt can be used to sample all channels at the end
> > > > of conversion and is optional.
> > > >
> > > > Signed-off-by: Paul Kocialkowski <paulk@sys-base.io>
> > > Hi Paul,
> > >
> > > Various comments inline. Most significant is that this seems to be
> > > suitable for a simple dataready trigger that will make your various
> > > interrupt and non interrupt flows more similar.
> >
> > And thanks for the fast review and insightful comments!
> >
> > I considered implementing a trigger in the driver, but the issue I found
> > is that the trigger is expected to be called from hard irq context,
> > while the new values are read in the bottom half.
>
> The trigger can be called from either the hard irq context or from
> a thread. See iio_trigger_poll_nested()
> There is a quirk that you then don't end up calling the registered
> hard irq handler for the trigger so sometimes a bit of fiddly code
> is needed to ensure timestamps etc are grabbed. Not sure that matters
> here.
>
If the timestamps do matter: here is a (maybe relevant?) discussion for
an issue I faced with timestamps for a driver that supports both FIFO
and triggered buffer mode
Please note that iio_trigger_poll_nested() was called
iio_trigger_poll_chained() back in that discussion.
https://lore.kernel.org/linux-iio/Y+6QoBLh1k82cJVN@carbian/
> > I understand the triggered
> > buffer callbacks are executed as a thread as well, so there would be race
> > between the two which could result in previous values being returned.
>
> With the above nested call it is all run in the same thread
> See handle_nested_irq() in particular the function docs.
> https://elixir.bootlin.com/linux/v6.12.1/source/kernel/irq/chip.c#L459
>
> > So I concluded that it was more beneficial to preserve the synchronous reading
> > mechanism over implementing the trigger.
>
> Definite preference for a trigger approach, but I may well still be missing
> a detail.
--
Kind Regards
Mehdi Djait
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 2/2] iio: light: Add support for the TI OPT4048 color sensor
2024-12-02 11:06 ` Jonathan Cameron
2024-12-02 14:55 ` Mehdi Djait
@ 2024-12-04 14:48 ` Paul Kocialkowski
1 sibling, 0 replies; 10+ messages in thread
From: Paul Kocialkowski @ 2024-12-04 14:48 UTC (permalink / raw)
To: Jonathan Cameron
Cc: Jonathan Cameron, linux-iio, devicetree, linux-kernel,
Lars-Peter Clausen, Rob Herring, Krzysztof Kozlowski,
Conor Dooley
[-- Attachment #1: Type: text/plain, Size: 34086 bytes --]
Hi Jonathan,
Le Mon 02 Dec 24, 11:06, Jonathan Cameron a écrit :
> On Sun, 1 Dec 2024 18:56:30 +0100
> Paul Kocialkowski <paulk@sys-base.io> wrote:
>
> > Hi Jonathan,
> >
> > Le Sun 01 Dec 24, 11:55, Jonathan Cameron a écrit :
> > > On Sat, 30 Nov 2024 18:42:12 +0100
> > > Paul Kocialkowski <paulk@sys-base.io> wrote:
> > >
> > > > The Texas Instruments OPT4048 is a XYZ tristimulus color sensor,
> > > > with an additional wide (visible + IR) channel.
> > > >
> > > > This driver implements support for all channels, with configurable
> > > > integration time and auto-gain. Both direct reading and
> > > > triggered-buffer modes are supported.
> > > >
> > > > Note that the Y channel is also reported as a separate illuminance
> > > > channel, for which a scale is provided (following the datasheet) to
> > > > convert it to lux units. Falling and rising thresholds are supported
> > > > for this channel.
> > > >
> > > > The device's interrupt can be used to sample all channels at the end
> > > > of conversion and is optional.
> > > >
> > > > Signed-off-by: Paul Kocialkowski <paulk@sys-base.io>
> > > Hi Paul,
> > >
> > > Various comments inline. Most significant is that this seems to be
> > > suitable for a simple dataready trigger that will make your various
> > > interrupt and non interrupt flows more similar.
> >
> > And thanks for the fast review and insightful comments!
> >
> > I considered implementing a trigger in the driver, but the issue I found
> > is that the trigger is expected to be called from hard irq context,
> > while the new values are read in the bottom half.
>
> The trigger can be called from either the hard irq context or from
> a thread. See iio_trigger_poll_nested()
> There is a quirk that you then don't end up calling the registered
> hard irq handler for the trigger so sometimes a bit of fiddly code
> is needed to ensure timestamps etc are grabbed. Not sure that matters
> here.
Oh great, I didn't know about it! I will definitely give it a try and respin
with trigger support.
> > I understand the triggered
> > buffer callbacks are executed as a thread as well, so there would be race
> > between the two which could result in previous values being returned.
>
> With the above nested call it is all run in the same thread
> See handle_nested_irq() in particular the function docs.
> https://elixir.bootlin.com/linux/v6.12.1/source/kernel/irq/chip.c#L459
>
> > So I concluded that it was more beneficial to preserve the synchronous reading
> > mechanism over implementing the trigger.
>
> Definite preference for a trigger approach, but I may well still be missing
> a detail.
I'll let you know if I run into trouble trying the trigger approach.
I also agree it is preferable.
> Jonathan
>
> >
> > But maybe I missed/misunderstood something here.
> >
> > > Jonathan
> > >
> > > > diff --git a/drivers/iio/light/Makefile b/drivers/iio/light/Makefile
> > > > index 321010fc0b93..f2031e6236f9 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_OPT4048) += opt4048.o
> > > > obj-$(CONFIG_PA12203001) += pa12203001.o
> > > > obj-$(CONFIG_ROHM_BU27008) += rohm-bu27008.o
> > > > obj-$(CONFIG_ROHM_BU27034) += rohm-bu27034.o
> > > > diff --git a/drivers/iio/light/opt4048.c b/drivers/iio/light/opt4048.c
> > > > new file mode 100644
> > > > index 000000000000..1ad5e6586aad
> > > > --- /dev/null
> > > > +++ b/drivers/iio/light/opt4048.c
> > > > @@ -0,0 +1,1145 @@
> > > > +// SPDX-License-Identifier: GPL-2.0-only
> > > > +/*
> > > > + * Copyright 2024 Paul Kocialkowski <paulk@sys-base.io>
> > > > + */
> > > > +
> > > > +#include <linux/delay.h>
> > > > +#include <linux/i2c.h>
> > > > +#include <linux/iio/events.h>
> > > > +#include <linux/iio/iio.h>
> > > > +#include <linux/iio/trigger_consumer.h>
> > > > +#include <linux/iio/triggered_buffer.h>
> > > > +#include <linux/log2.h>
> > > > +#include <linux/module.h>
> > > > +#include <linux/pm_runtime.h>
> > > > +#include <linux/regulator/consumer.h>
> > > > +
> > > > +#define OPT4048_CH0_DATA0 0x0
> > > > +#define OPT4048_CH0_DATA1 0x1
> > > > +#define OPT4048_CH1_DATA0 0x2
> > > > +#define OPT4048_CH1_DATA1 0x3
> > > > +#define OPT4048_CH2_DATA0 0x4
> > > > +#define OPT4048_CH2_DATA1 0x5
> > > > +#define OPT4048_CH3_DATA0 0x6
> > > > +#define OPT4048_CH3_DATA1 0x7
> > > > +
> > > > +#define OPT4048_CH_DATA0_MSB_VALUE(v) ((v) & GENMASK(11, 0))
> > > > +#define OPT4048_CH_DATA0_EXPONENT_VALUE(v) (((v) & GENMASK(15, 12)) >> 12)
> > >
> > > For all these - just define the masks and use FIELD_GET() / FIELD_PREP()
> > > inline. Masks should be defined with names that make it clear what they are
> > > masking and in what registers.
> >
> > I see. Note that I generally tried to follow the datasheet's terminology for
> > convenience.
>
> Understood, but you can stay near to that with well named masks etc
> and generally it remains pretty readable and compact.
Sure, that should work out.
>
>
> >
> > > > +};
> > > > +
> > > > +struct opt4048_sensor {
> > > > + struct device *dev;
> > > > + struct i2c_client *i2c_client;
> > > > + struct iio_dev *iio_dev;
> > > > +
> > > > + struct regulator *vdd_supply;
> > > > +
> > > > + struct opt4048_sensor_state state;
> > >
> > > > + struct opt4048_sensor_scan scan;
> > >
> > > As below. I think you should just always do the read in the trigger handler.
> >
> > As far as I could see the hardware doesn't guarantee that values come from
> > the same read sequence (even if read as a single i2c transaction). This is why
> > my approach was to read them as early as possible after the irq is signaled
> > and cache them here, instead of doing the actual read in the trigger handler
> > or sysfs read callback (which may have both values from the previous and next
> > reads).
>
> As above, if you do it via handle_nested_irq, it ends up being very nearly the
> same time as you have it here (a few calls later).
>
> >
> > The only thing the hardware guarantees is that the DATA1 value is latched when
> > DATA0 is read.
> Ah good. So no tearing at least.
>
> Does it actually matter if you get reads from neighbouring samples? Light sensors
> tend not to be measuring anything particularly dynamic. I doubt you'll have problems
> anyway, but if you do I'm not sure I see it as mattering.
I've noticed that the issue tends to be noticeable with long integration times
and moving the sensor between two objects. Typically the first channel would
match the next object and the two others the previous one while the next reading
had all three values coherent. That was when reading samples at a random time
(typically triggered with hrtimer).
However it appears that the sensor only has a single ADC so I'm not even quite
sure the readings are taken simultaneously on all channels (although that might
still be the case).
Either way, reading values in the interrupt handler helps and I want to make
sure I'm doing everything possible to get "as coherent as possible" values.
> >
> > > > + bool scan_sync;
> > >
> > > This needs a comment. Interrupts and sync aren't something that normally goes
> > > together so the name is somewhat confusing.
> >
> > Okay. The idea is to have a synchronous read where all channels are read as
> > soon as conversion done is signaled.
>
> That's not synchronous though, as they are done on an async interrupt.
> So maybe thing of a different term.
I meant "synchronous with the irq" but yeah I guess you're right that it's not
very clear. Maybe "scan_irq_ready" would be better.
> > > > +
> > > > + /* Report illuminance using Y intensity value. */
> > >
> > > That seems 'interesting'. Why?
> >
> > The Y channel is defined by CIE1931 to match the (photopic) illuminance response
> > curve. The main difference is that Y is unit-less while illuminance is defined
> > in lux. This specific channel gets an extra scale property to report the
> > conversion coefficient for lux.
>
> Ah I had it in my head that the curves were similar but not the same.
> Fair enough.
>
> >
> > However we still need the Y value to be reported as such in order to be related
> > to X and Z.
> >
> > > > + scan->channels[4] = scan->channels[1];
> > > > +
> > > > + return 0;
> > > > +}
> > > > +
> > > > +static int opt4048_identify(struct opt4048_sensor *sensor)
> > > > +{
> > > > + struct device *dev = sensor->dev;
> > > > + int ret;
> > > > + u16 id, low, high;
> > > > +
> > > > + ret = pm_runtime_resume_and_get(dev);
> > > > + if (ret < 0)
> > > > + return ret;
> > > > +
> > > > + ret = i2c_smbus_read_word_swapped(sensor->i2c_client, OPT4048_DID);
> > > > + if (ret < 0)
> > > > + goto complete;
> > > > +
> > > > + low = (u16)OPT4048_DID_L_VALUE(ret);
> > > > + high = (u16)OPT4048_DID_H_VALUE(ret);
> > > > +
> > > > + id = OPT4048_DID_VALUE(low, high);
> > > > +
> > > > + switch (id) {
> > > > + case OPT4048_DID_OPT4048:
> > > > + dev_info(dev, "identified OPT4048 sensor\n");
> > > This isn't useful information and can be easily established once the
> > > driver has loaded. So dev_dbg() at most.
> >
> > Honestly I see a bunch of driver doing that (especially camera sensors)
> > and find it generally useful. But I don't mind getting rid of it.
> I try to squash them. Spend too long staring at enormously long kernel
> boot logs :) I'm fine if it provides extra info though. Some devices have
> serial numbers or firmware version numbers etc.
No extra info here. I guess I'll just get rid of it.
> >
> > > > + ret = 0;
> > > > + break;
> > > > + default:
> > > > + dev_err(dev, "unknown sensor with id: %#x\n", id);
> > > > + ret = -ENODEV;
> > > We shouldn't treat a failure to match the ID as a reason to fail probe.
> > > Consider the use of fallback IDs in device tree with older kernels.
> > > If a new device comes along that is backwards compatible, we want that
> > > to work with out driver modifications.
> >
> > Honestly this is really just a way to check that i2c transaction are working,
> > that the device is actually there and to report when it's not.
> >
> > I'm not sure it's such a good idea to assume that other IDs would not be
> > a sign that something very wrong is going on (typically some other device
> > connected on the same i2c address). Failing to probe will prevent
> > misconfiguring another device.
> >
> > Just adding another case here if a new device is ever introduced doesn't
> > sound like such a big drawback in comparison.
>
> I used to agree with you, but view of DT maintainers if that if you have a broken
> board or device or a wrong firmware then it isn't the bindings or Linux's problem
> to solve. Given we have a 'lot' of fallback compatibles now I'm now pretty
> convinced on this.
>
> An info message should be enough to show something is odd and give the breadcrumbs
> needed to debug.
Okay, not sure I'm very convinced but I'll follow your indications.
>
> >
> > > It's fine to print an dev_info message though so the user is aware
> > > that we are ignoring the missmatch.
> > >
> > > > + break;
> > > > + }
> > > > +
> > > > +complete:
> > > > + pm_runtime_put_sync(dev);
> > > > +
> > > > + return ret;
> > > > +}
> > > > +
> > > > +static int opt4048_power(struct opt4048_sensor *sensor, bool on)
> > > > +{
> > > > + struct opt4048_sensor_state *state = &sensor->state;
> > > > + int ret;
> > > > +
> > > > + mutex_lock(&sensor->lock);
> > > guard()
> > > > +
> > > > + state->active = on;
> > > > + ret = opt4048_state_configure_cfg0(sensor);
> > > > +
> > > > + mutex_unlock(&sensor->lock);
> > > > +
> > > > + return ret;
> > > > +}
> > >
> > > > +static int opt4048_state_configure_cfg1(struct opt4048_sensor *sensor)
> > > > +{
> > > > + u16 value;
> > > > + int ret;
> > > I don't really see any advantage in these wrappers. I'd just have
> > > the code inline in opt4048_state_configure()
> >
> > Well I do think they are important to make the code more digest.
>
> As someone who digested it. They don't make it easier. They just
> make me scroll backwards and forwards a lot whilst reviewing.
I don't mind squashing most, but I'd at least keep opt4048_state_configure_cfg0
which needs to be called separately from opt4048_power.
> >
> > > > +
> > > > + /* Assign threshold to the Y channel for illuminance. */
> > > > + value = OPT4048_CFG1_I2C_BURST |
> > > > + OPT4048_CFG1_INT_DIR_OUT |
> > > > + OPT4048_CFG1_THRESHOLD_CH_SEL(1) |
> > > > + OPT4048_CFG1_RESERVED;
> > >
> > > combine these on fewer lines. Just generally stay below 80 chars unless
> > > there is a strong readability reason to go longer.
> >
> > I find it a lot more readable with one item per line.
>
> Hmm. In this particular case I don't mind that much.
> It is common for these to be come stupidly long for no real readability
> advantage.
>
> >
> > > > +
> > > > + if (sensor->scan_sync)
> > > > + value |= OPT4048_CFG1_INT_CFG_DATA_READY_ALL;
> > > > + else
> > > > + value |= OPT4048_CFG1_INT_CFG_ALERT;
> > > > +
> > > > + ret = i2c_smbus_write_word_swapped(sensor->i2c_client, OPT4048_CFG1,
> > > > + value);
> > > > + if (ret < 0)
> > > > + return ret;
> > > > +
> > > > + return 0;
> > > > +}
>
> > > > +static int opt4048_iio_read_raw(struct iio_dev *iio_dev,
> > > > + struct iio_chan_spec const *channel,
> > > > + int *value_first, int *value_second, long mask)
> > > > +{
> > > > + struct opt4048_sensor *sensor = iio_priv(iio_dev);
> > > > + struct opt4048_sensor_state *state = &sensor->state;
> > > > + struct device *dev = sensor->dev;
> > > > + int ret;
> > > > +
> > > > + switch (mask) {
> > > > + case IIO_CHAN_INFO_RAW:
> > > Need define scope {}
> > >
> > > > + unsigned int scan_index;
> > > > +
> > > > + ret = iio_device_claim_direct_mode(iio_dev);
> > > > + if (ret)
> > > > + return ret;
> > > > +
> > > > + ret = pm_runtime_resume_and_get(dev);
> > > > + if (ret < 0)
> > > > + goto release_direct_mode;
> > > > +
> > > > + if (sensor->scan_sync) {
> > >
> > > So this is curious. You just power up sensor and let it run until interrupt.
> > > I'd expect to have interrupt disabled unless buffered capture is in use.
> > > It would be fine to turn it on for a single cycle though if that makes capturing
> > > on demand simpler.
> >
> > So that reflects the fact that getting synchronous readings from the sensor
> > does require reading the channels "soon after" the interrupt.
>
> For the direct read it is fine to grab them in the interrupt. Just run
> it with direct mode claimed an a check in your interrupt handler on whether
> the buffer is enabled. iio_device_claim_direct_mode() will prevent any
> transitions happening into buffered mode concurrent with this single shot
> read.
>
> >
>
>
> > > > +static int opt4048_iio_write_event_value(struct iio_dev *iio_dev,
> > > > + struct iio_chan_spec const *channel,
> > > > + enum iio_event_type type,
> > > > + enum iio_event_direction direction,
> > > > + enum iio_event_info info,
> > > > + int value_first, int value_second)
> > > > +{
> > > > + struct opt4048_sensor *sensor = iio_priv(iio_dev);
> > > > + struct opt4048_sensor_state *state = &sensor->state;
> > > > + u32 value;
> > > > + int ret;
> > > > +
> > > > + switch (direction) {
> > > > + case IIO_EV_DIR_RISING:
> > > > + value = (u32)value_first;
> > > > + opt4048_threshold_convert(value, state->threshold_high);
> > > > + break;
> > > > +
> > > > + case IIO_EV_DIR_FALLING:
> > > > + value = (u32)value_first;
> > > > + opt4048_threshold_convert(value, state->threshold_low);
> > > > + break;
> > > > +
> > > > + default:
> > > > + return -EINVAL;
> > > > + }
> > > > +
> > > > + if (pm_runtime_suspended(sensor->dev))
> > > > + return 0;
> > > I'm curious. Why isn't this an error return? Check for other cases of this.
> > > I think they should all return an error so userspace knows something unexpected
> > > is going on.
> >
> > I'd expect userspace to be able to configure the threshold without anything
> > reading values at that moment. The point is to keep the sensor off.
>
> Ah. I misread this. How is the race prevented with it being enabled
> at this point?
Not sure why that would need anything special. Threshold could be configured
with the sensor on and will take effect next time the channels are ready.
So maybe one situation would be that we return from here and have the irq hit
right after it, which would be too late to take the new threshold into account.
Not sure what we could really do about it and honestly I don't particularly care
if it takes one reading of latency to apply the threshold.
> >
> > > > +
> > > > + ret = opt4048_state_configure_threshold(sensor);
> > > > + if (ret)
> > > > + return ret;
> > >
> > > return opt...
> > >
> > > > +
> > > > + return 0;
> > > > +}
>
> > > > +
> > > > + switch (direction) {
> > > > + case IIO_EV_DIR_RISING:
> > > > + state->threshold_high_active = !!active;
> > >
> > > This is why the signature became bool. All drivers had the same handling
> > > which made no sense :(
> > >
> > > FWIW you don't need the !! even before that change.
> >
> > Yeah bool isn't really a thing in C but it just feels more consistent.
> > It would be necessary if I had defined threshold_high_active as a single-bit
> > field though.
> Don't do that. Looses the clear meaning as a true / false field.
> In kernel c bool is well defined (and in more recent C specs).
>
> >
> > >
> > >
> > > > + break;
> > > > +
> > > > + case IIO_EV_DIR_FALLING:
> > > > + state->threshold_low_active = !!active;
> > > > + break;
> > > > +
> > > > + default:
> > > > + ret = -EINVAL;
> > > When using guard. Can just return -EINVAL;
> > > > + goto complete;
> > > > + }
> > > > +
> > > > + if (pm_runtime_suspended(sensor->dev))
> > > > + goto complete;
> > > As above - direct return.
> > > Though fun question of what you should do if this does fail as
> > > we are then in a weird unknown state.
> > > why not an error return?
> >
> > It's really not an error if it fails and the state handling is written to allow
> > configuring it with the device off. It just updates the states without reaching
> > the device and the state gets applied at the next resume.
>
> I'd misread the code. Feels like there is a race here however.
Yeah I guess just like above there could be one reading of latency in the worst
case.
>
> >
> > > > +
> > > > + ret = opt4048_state_configure_threshold(sensor);
> > > Will become
> > > return opt4048_*
> > > > +
> > > > +complete:
> > > > + mutex_unlock(&sensor->lock);
> > > > +
> > > > + return ret;
> > > > +}
> > > };
> > > > +
> > > > +static irqreturn_t opt4048_iio_buffer_trigger(int irq, void *data)
> > > > +{
> > > > + struct iio_poll_func *poll_func = data;
> > > > + struct iio_dev *iio_dev = poll_func->indio_dev;
> > > > + struct opt4048_sensor *sensor = iio_priv(iio_dev);
> > > > + struct opt4048_sensor_scan scan = { 0 };
> > > > + s64 timestamp;
> > > > + unsigned int index = 0;
> > > > + unsigned int i;
> > > > + int ret;
> > > > +
> > > > + /* Capture timestamp just before reading values. */
> > > > + timestamp = iio_get_time_ns(iio_dev);
> > > > +
> > > > + mutex_lock(&sensor->lock);
> > > > +
> > > > + if (!sensor->scan_sync) {
> > > > + ret = opt4048_data_scan(sensor, &sensor->scan);
> > > So you have a weird hybrid of capture in the data ready interrupt and here.
> >
> > The point is to allow both interrupt-based and interrupt-less operation here.
> >
> > > Why not just kick this off by having a data ready trigger and
> > > using iio_trigger_poll() to effectively call this on the data ready
> > > interrupt or on an other trigger. That way should need no special handling
> > > for your sync scan.
> >
> > See the comment above, maybe I missed something though.
>
> I think you do need a path for an early read, but that doesn't stop you
> doing simple triggered capture in the trigger handler (allowing multiple types
> of trigger). Just need to fix the mode so we don't enter buffered mode (see above
> stuff on claiming direct mode) then check for whether the buffer is enabled
> here. Grab a copy it if it isn't.
Not sure what you mean here and what's wrong with this current design.
I essentially return the values last captured from the irq handler here or
do the reading if irq is not available.
> >
> > >
> > >
> > > > + if (ret)
> > > > + goto complete;
> > > > + }
> > > > +
> > > > +static const struct iio_buffer_setup_ops opt4048_iio_buffer_setup_ops = {
> > > > + .preenable = opt4048_iio_buffer_preenable,
> > > > + .postdisable = opt4048_iio_buffer_postdisable,
> > > > +};
> > > > +
> > > > +static int opt4048_iio_setup(struct opt4048_sensor *sensor)
> > > This function doesn't add much. I'd just put the code inline in prbe.
> >
> > I just like splitting framework-specific init, it makes things more readable.
> I'm not convinced. I wouldn't mind you doing some sub steps such
> as buffer setup, trigger setup etc.
>
> Maybe it's because it is the ordering in that code that goes wrong most
> often + I'd rather all the stuff was grouped by functionality, not by
> subsystem. So register trigger than get interrupt etc. Similar with buffer.
> Then finally see that the exposure of sysfs controls is done in
> probe()
Well I still fail to see why this is a problem. I agree that splitting into
sub-functions may make it easier to introduce issues about init ordering, but
that's not the case in my driver. Feels like I'm being punished for something
I didn't do.
> >
> > > > +{
> > > > + struct iio_chan_spec *channels;
> > > > + struct iio_dev *iio_dev = sensor->iio_dev;
> > > Just pass it the iio_dev in.
> >
> > My taste is rather to have a device-wide structure that is used in all
> > functions that are not callbacks from the framework/bus/device.
>
> We have had a lot of broken code via sensor->iio_dev type
> code in the past. I'm not going down that route again so
> no to having a sensor->iio_dev.
>
> Structures should have clean nesting. It can be done either way
> around - but it's a case of pick one and stick to it.
>
> 1. Subsystem structure inside the device specific one.
> 2. Device specific one inside the subsystem one.
>
> IIO does 2. So I don't want to see mixing and matching the two
> models.
Well again it feels like I'm being punished for other people's mistakes. Having
such bi-directional links is not constitutive of bad design and most subsystems
don't really care as long as nothing gets broken along the way.
I do understand it makes it easier for you to review/maintain things if all
drivers follow the same pattern but frankly this is unlikely to be touched by
future changes to the driver so I don't really see why this would become an
issue.
>
>
> >
> > > > + struct device *dev = sensor->dev;
> > > > + unsigned int channels_count;
> > > > + int ret;
> > > > +
> > > > + channels_count = ARRAY_SIZE(opt4048_iio_channels);
> > > > +
> > > > + if (sensor->i2c_client->irq > 0) {
> > > > + channels = devm_kzalloc(dev, sizeof(opt4048_iio_channels),
> > > > + GFP_KERNEL);
> > >
> > > kmemdup. However I'd rather just see this picking between two static
> > > const arrays of channels. There are only two cases so it isn't worth dynamic
> > > channel setup complexity.
> >
> > Well they would be 99 % identical. I don't really see how this would be a
> > beneficial duplication versus the low complexity of these few lines.
>
> Generally static const data ends up cleaner in the long run, but sure
> we can refactor when it becomes more complex.
>
> A lot of drivers become very very complex in this regard and we end
> up ripping out code like this a lot in favor of const data.
I understand the general idea but I believe it doesn't fit well with this
particular case.
>
>
>
> > > Comment doesn't add anything that isn't obvious from the code.
> > > So don't have it.
> >
> > These are meant as categories to make it clear what the following code is
> > related to.
>
> Sure. They seem like a good idea when people write new drivers and then we
> have to go repair them when they break. They do that far too often.
> Hence, preference to not have them in the first place.
> Don't need a comment about irq to tell a function that is well named
> like this one is about irqs!
Okay and I guess it's not such a huge driver either so maybe I overdid it a bit.
> >
> > > > +
> > > > +static irqreturn_t opt4048_irq(int irq, void *data)
> > > > +{
> > > > + struct opt4048_sensor *sensor = data;
> > > > + struct opt4048_sensor_state *state = &sensor->state;
> > > > + struct iio_dev *iio_dev = sensor->iio_dev;
> > >
> > > Set data = iio_dev and then use iio_priv() on that.
> > > There shouldn't be a need to go back the other way.
> >
> > Not really my taste, sorry.
>
> See above. Very strong preference for not mixing and matching
> styles.
>
>
>
>
> > > > +
> > > > + state->status = status;
> > > Add a comment on why you are saving this (I guess because the device
> > > interrupts every time otherwise). Does it interrupt on the reverse
> > > direction? If not how is this cleared?
> >
> > Ah I thought it was clear from the fact that we use it in the statements above
> > to find out if we just crossed a threshold.
> >
> > The device reports "value above/below threshold" at every reading and not
> > "threshold crossed" so I need to compare with previous state to derive
> > rising/falling.
>
> Ok. So it interrupts every time out of the window. That's horrible.
Yes. Also the threshold cannot really be disabled outside of setting it to
the min/max, which adds another coating of horrible.
> Maybe add a comment on that. I was kind of assuming it didn't interrupt again,
> just that the status bits were set.
Will do!
>
>
>
> > > > +
> > > > +static int opt4048_suspend(struct device *dev)
> > > > +{
> > > > + struct opt4048_sensor *sensor = dev_get_drvdata(dev);
> > > > + int ret;
> > > > +
> > > > + ret = opt4048_power(sensor, 0);
> > > > + if (ret)
> > > > + goto error;
> > > > +
> > > > + ret = regulator_disable(sensor->vdd_supply);
> > > > + if (ret)
> > > > + goto error;
> > > > +
> > > > + return 0;
> > > > +
> > > > +error:
> > > > + return -EAGAIN;
> > > Direct returns preferred over both eating the error codes and
> > > a goto like this.
> >
> > Hehe the suspend callback doesn't work that way. If you *ever* return something
> > else than -EAGAIN, it will never attempt to resume the device again. This is
> > documented in the runtime pm semantics.
> >
> > So it is very crucial to eat the error code and return -EAGAIN if we want to
> > have a chance at it again. Many drivers get this wrong.
>
> There is an argument that if you fail in any of the above calls the state
> is completely unknown and the right thing to do is never resume. It's
> a case of cross your fingers and hope.
Actually one thing that happened to me was a loose wire that didn't connect well
in all physical positions. So at one point an i2c transaction failed in resume
and it forbid me from ever using the sensor again without reboot when I returned
that error code. I don't think I/O errors should be *that* fatal.
Yes the state might be a bit messed up but it will be sorted out at the next
(successful) resume.
> >
> > > > +}
> > > > +
> > > > +static int opt4048_resume(struct device *dev)
> > > > +{
> > > > + struct opt4048_sensor *sensor = dev_get_drvdata(dev);
> > > > + unsigned long sleep_min;
> > > > + unsigned int index;
> > > > + int ret;
> > > > +
> > > > + ret = regulator_enable(sensor->vdd_supply);
> > > > + if (ret)
> > > > + goto error;
> > > > +
> > > > + /* Wait for the regulator to settle and the chip to power-on. */
> > > > + udelay(30);
> > > > +
> > > > + ret = opt4048_state_configure(sensor);
> > > > + if (ret)
> > > > + goto error_regulator;
> > > > +
> > > > + ret = opt4048_power(sensor, 1);
> > > > + if (ret)
> > > > + goto error_regulator;
> > > > +
> > > > + /* Wait for conversion to be ready for all channels. */
> > > We might not have powered up for that reason but I guess this does
> > > little harm.
> >
> > By design this driver only powers up the device for readings. Everything else
> > is kept staging in the state structure.
>
> I'd misread some of the earlier code. Thanks for the clarification.
>
> >
> > > > + index = 2 * sensor->state.conversion_time_index + 1;
> > > > + sleep_min = opt4048_conversion_time_available[index] * 4;
> > > > +
> > > > + usleep_range(sleep_min, 5 * sleep_min / 4);
> > >
> > > Perhaps use fsleep() to avoid anyone needing to reason about the margins
> > > etc.
> > >
> > > > +
> > > > + return 0;
> > > > +
> > > > +error_regulator:
> > > > + regulator_disable(sensor->vdd_supply);
> > > > +
> > > > +error:
> > > Direct returns make for easier to review code as following an error
> > > path doesn't require checking to see what the cleanup is.
> >
> > I prefer it over duplication.
>
> Hmm. We disagree on this. What I see is code where I have to go
> check the error handler on paths where it is completely pointless as
> it's not doing anything and I could have just seen the return at
> the goto.
Just to clarify here, do you also mean that I should duplicate the
regulator_disable in error checking conditions, or just avoid this error label
for the regulator_enable call?
> >
> > > > + return -EAGAIN;
> > > > +}
> > > > +
> > > > +static const struct dev_pm_ops opt4048_pm_ops = {
> > > > + .runtime_suspend = opt4048_suspend,
> > > > + .runtime_resume = opt4048_resume,
> > > > +};
> > > > +
> > > > +/* I2C */
> > > Not a particularly useful comment. I'd drop it.
> > > > +
> > > > +static int opt4048_probe(struct i2c_client *client)
> > > > +{
> > > > + struct device *dev = &client->dev;
> > > > + struct opt4048_sensor *sensor;
> > > > + struct iio_dev *iio_dev;
> > > > + int irq = client->irq;
> > > > + int ret;
> > > > +
> > > > + iio_dev = devm_iio_device_alloc(dev, sizeof(*sensor));
> > > > + if (!iio_dev)
> > > > + return -ENOMEM;
> > > > +
> > > > + sensor = iio_priv(iio_dev);
> > > > +
> > > > + sensor->dev = dev;
> > > > + sensor->i2c_client = client;
> > > > + sensor->iio_dev = iio_dev;
> > > That is almost always a sign that you have a less than ideal layering
> > > in the driver.
> >
> > I disagree, my preference is to have a top-level device-specific structures
> > that holds everything else and is used in all local functions. It find it
> > a lot more convenient and readable.
>
> See above. Whilst we are going to disagree on this, I'm going to hold firm
> because I do not want other driver authors copying this pattern. It makes
> my job as a reviewer harder because there are two ways to do things when
> there should only be one.
See my response above. I'm inclined to do as you say but I don't really find
it very fair.
> >
> > > > +
> > > > + i2c_set_clientdata(client, sensor);
> > > > +
> > > > + mutex_init(&sensor->lock);
> > > ret = devm_mutex_init()
> > > if (ret)
> > > return ret;
> > > > +
> > > > + sensor->vdd_supply = devm_regulator_get(dev, "vdd");
> > >
> > > Given runtime PM may not even be enabled, you should turn the power on.
> > > Unconditionally.
> >
> > The driver definitely depends on runtime PM being enabled (and I think it's
> > a good thing to keep the device off when it's not used).
>
> Generally a driver should depend on runtime_pm. What in here makes
> that necessary (rather than a good idea for power saving!)?
Maybe you meant shouldn't? There's actually more and more subsystems that
require using it instead of manual power management or always-on approaches.
I don't see any downside in using it and it makes life a lot easier in many
regards. And I doubt there's much interest in building kernels that don't
enable it at all nowadays, just because it's used all around the place.
In addition to code factoring and features (such as autosuspend, which is really
nice), I don't think power savings are just a detail. They matter a lot in many
situations and having power to spare doesn't mean it's a good thing to waste
that power.
Lastly the kernel was notorious for being bad at power management for a very
long time. Now that we have all the tools to resolve this situation it feels
a bit silly to me not to do it.
> If it does depend on it, then require it via kconfig depends.
> But be prepared for push back on that as it shouldn't be necessary.
Oh it should definitely be a kconfig dependency. It's my mistake if I didn't
specify it.
Cheers,
Paul
--
Paul Kocialkowski,
Independent contractor - sys-base - https://www.sys-base.io/
Free software developer - https://www.paulk.fr/
Expert in multimedia, graphics and embedded hardware support with Linux.
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 2/2] iio: light: Add support for the TI OPT4048 color sensor
2024-12-02 14:55 ` Mehdi Djait
@ 2024-12-04 14:49 ` Paul Kocialkowski
0 siblings, 0 replies; 10+ messages in thread
From: Paul Kocialkowski @ 2024-12-04 14:49 UTC (permalink / raw)
To: Mehdi Djait
Cc: Jonathan Cameron, Jonathan Cameron, linux-iio, devicetree,
linux-kernel, Lars-Peter Clausen, Rob Herring,
Krzysztof Kozlowski, Conor Dooley
[-- Attachment #1: Type: text/plain, Size: 3552 bytes --]
Hi Mehdi,
Glad to see you active here :)
Le Mon 02 Dec 24, 15:55, Mehdi Djait a écrit :
> Hello Paul :)
>
> On Mon, Dec 02, 2024 at 11:06:59AM +0000, Jonathan Cameron wrote:
> > On Sun, 1 Dec 2024 18:56:30 +0100
> > Paul Kocialkowski <paulk@sys-base.io> wrote:
> >
> > > Hi Jonathan,
> > >
> > > Le Sun 01 Dec 24, 11:55, Jonathan Cameron a écrit :
> > > > On Sat, 30 Nov 2024 18:42:12 +0100
> > > > Paul Kocialkowski <paulk@sys-base.io> wrote:
> > > >
> > > > > The Texas Instruments OPT4048 is a XYZ tristimulus color sensor,
> > > > > with an additional wide (visible + IR) channel.
> > > > >
> > > > > This driver implements support for all channels, with configurable
> > > > > integration time and auto-gain. Both direct reading and
> > > > > triggered-buffer modes are supported.
> > > > >
> > > > > Note that the Y channel is also reported as a separate illuminance
> > > > > channel, for which a scale is provided (following the datasheet) to
> > > > > convert it to lux units. Falling and rising thresholds are supported
> > > > > for this channel.
> > > > >
> > > > > The device's interrupt can be used to sample all channels at the end
> > > > > of conversion and is optional.
> > > > >
> > > > > Signed-off-by: Paul Kocialkowski <paulk@sys-base.io>
> > > > Hi Paul,
> > > >
> > > > Various comments inline. Most significant is that this seems to be
> > > > suitable for a simple dataready trigger that will make your various
> > > > interrupt and non interrupt flows more similar.
> > >
> > > And thanks for the fast review and insightful comments!
> > >
> > > I considered implementing a trigger in the driver, but the issue I found
> > > is that the trigger is expected to be called from hard irq context,
> > > while the new values are read in the bottom half.
> >
> > The trigger can be called from either the hard irq context or from
> > a thread. See iio_trigger_poll_nested()
> > There is a quirk that you then don't end up calling the registered
> > hard irq handler for the trigger so sometimes a bit of fiddly code
> > is needed to ensure timestamps etc are grabbed. Not sure that matters
> > here.
> >
>
> If the timestamps do matter: here is a (maybe relevant?) discussion for
> an issue I faced with timestamps for a driver that supports both FIFO
> and triggered buffer mode
>
> Please note that iio_trigger_poll_nested() was called
> iio_trigger_poll_chained() back in that discussion.
>
> https://lore.kernel.org/linux-iio/Y+6QoBLh1k82cJVN@carbian/
Thanks for the hint! I'll definitely look it up for details.
Cheers,
Paul
> > > I understand the triggered
> > > buffer callbacks are executed as a thread as well, so there would be race
> > > between the two which could result in previous values being returned.
> >
> > With the above nested call it is all run in the same thread
> > See handle_nested_irq() in particular the function docs.
> > https://elixir.bootlin.com/linux/v6.12.1/source/kernel/irq/chip.c#L459
> >
> > > So I concluded that it was more beneficial to preserve the synchronous reading
> > > mechanism over implementing the trigger.
> >
> > Definite preference for a trigger approach, but I may well still be missing
> > a detail.
>
> --
> Kind Regards
> Mehdi Djait
--
Paul Kocialkowski,
Independent contractor - sys-base - https://www.sys-base.io/
Free software developer - https://www.paulk.fr/
Expert in multimedia, graphics and embedded hardware support with Linux.
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply [flat|nested] 10+ messages in thread
end of thread, other threads:[~2024-12-04 14:49 UTC | newest]
Thread overview: 10+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2024-11-30 17:42 [PATCH 1/2] dt-bindings: iio: Add TI OPT4048 color sensor bindings Paul Kocialkowski
2024-11-30 17:42 ` [PATCH 2/2] iio: light: Add support for the TI OPT4048 color sensor Paul Kocialkowski
2024-12-01 1:29 ` kernel test robot
2024-12-01 11:55 ` Jonathan Cameron
2024-12-01 17:56 ` Paul Kocialkowski
2024-12-02 11:06 ` Jonathan Cameron
2024-12-02 14:55 ` Mehdi Djait
2024-12-04 14:49 ` Paul Kocialkowski
2024-12-04 14:48 ` Paul Kocialkowski
2024-12-02 8:41 ` [PATCH 1/2] dt-bindings: iio: Add TI OPT4048 color sensor bindings Krzysztof Kozlowski
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).