[PATCH v4 1/2] dt-bindings: iio: light: ROHM BH1745

[PATCH v4 1/2] dt-bindings: iio: light: ROHM BH1745

[Mudit Sharma]

Add ROHM BH1745 - 4 channel I2C colour sensor's dt-bindings.

Signed-off-by: Mudit Sharma <muditsharma.info@gmail.com>
Reviewed-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
---
v3->v4:
- No changes
v2->v3:
- Move 'additionalProperties' after 'required' block
- Remove block style indicator '|' from description
v1->v2:
- Fix yaml issue: Make `maintainers` a list

 .../bindings/iio/light/rohm,bh1745.yaml       | 49 +++++++++++++++++++
 1 file changed, 49 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/iio/light/rohm,bh1745.yaml

diff --git a/Documentation/devicetree/bindings/iio/light/rohm,bh1745.yaml b/Documentation/devicetree/bindings/iio/light/rohm,bh1745.yaml
new file mode 100644
index 000000000000..d5338a0af6b3
--- /dev/null
+++ b/Documentation/devicetree/bindings/iio/light/rohm,bh1745.yaml
@@ -0,0 +1,49 @@
+# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/iio/light/rohm,bh1745.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: ROHM BH1745 colour sensor
+
+maintainers:
+  - Mudit Sharma <muditsharma.info@gmail.com>
+
+description:
+  BH1745 is an I2C colour sensor with red, green, blue and clear
+  channels. It has a programmable active low interrupt pin.
+  Interrupt occurs when the signal from the selected interrupt
+  source channel crosses set interrupt threshold high/low level.
+
+properties:
+  compatible:
+    const: rohm,bh1745
+
+  reg:
+    maxItems: 1
+
+  interrupts:
+    maxItems: 1
+
+required:
+  - compatible
+  - reg
+
+additionalProperties: false
+
+examples:
+  - |
+    #include <dt-bindings/interrupt-controller/irq.h>
+    i2c {
+        #address-cells = <1>;
+        #size-cells = <0>;
+
+        colour-sensor@38 {
+            compatible = "rohm,bh1745";
+            reg = <0x38>;
+            interrupt-parent = <&gpio>;
+            interrupts = <19 IRQ_TYPE_LEVEL_LOW>;
+        };
+    };
+
+...
-- 
2.43.0

[PATCH v4 2/2] iio: light: ROHM BH1745 colour sensor

[Mudit Sharma]

Add support for BH1745, which is an I2C colour sensor with red, green,
blue and clear channels. It has a programmable active low interrupt
pin. Interrupt occurs when the signal from the selected interrupt
source channel crosses set interrupt threshold high or low level.

This driver includes device attributes to configure the following:
- Interrupt pin latch: The interrupt pin can be configured to
  be latched (until interrupt register (0x60) is read or initialized)
  or update after each measurement.
- Interrupt source: The colour channel that will cause the interrupt
  when channel will cross the set threshold high or low level.

This driver also includes device attributes to present valid
configuration options/values for:
- Integration time
- Interrupt colour source
- Hardware gain

Add myself as the maintainer for this driver in MAINTAINERS.

Signed-off-by: Mudit Sharma <muditsharma.info@gmail.com>
Reviewed-by: Ivan Orlov <ivan.orlov0322@gmail.com>
Reviewed-by: Javier Carrasco <javier.carrasco.cruz@gmail.com>
---
v3->v4:
- Fix formatting:
  - Remove redundant new line
  - Use '0x' rather than '0X'
v2->v3:
- Squash commit for addition to MAINTAINERS
- Fix code style for consistency:
  - New line before last 'return'
  - Use variable name 'value' instead of 'val' in
    'bh1745_set_trigger_state()'
  - Align function parameters to match parenthesis
  - Avoid use of magic number
- Use named enum instead of anonymous enum
- Use 'guard(mutex)(&data->lock)' instead of 'mutex_lock()'
  'mutex_unlock()'
- Only initialize 'ret' and 'value' when necessary
- Fix and optimize logic for `in_interrupt_latch_store()` 
- Fix error handling in irq , trigger handlers and dev attribute for
  latch
v1->v2:
- No changes

 MAINTAINERS                |   6 +
 drivers/iio/light/Kconfig  |  12 +
 drivers/iio/light/Makefile |   1 +
 drivers/iio/light/bh1745.c | 863 +++++++++++++++++++++++++++++++++++++
 4 files changed, 882 insertions(+)
 create mode 100644 drivers/iio/light/bh1745.c

diff --git a/MAINTAINERS b/MAINTAINERS
index d6c90161c7bf..024c14738dc7 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -19407,6 +19407,12 @@ S:	Supported
 F:	drivers/power/supply/bd99954-charger.c
 F:	drivers/power/supply/bd99954-charger.h
 
+ROHM BH1745 COLOUR SENSOR
+M:	Mudit Sharma <muditsharma.info@gmail.com>
+L:	linux-iio@vger.kernel.org
+S:	Maintained
+F:	drivers/iio/light/bh1745.c
+
 ROHM BH1750 AMBIENT LIGHT SENSOR DRIVER
 M:	Tomasz Duszynski <tduszyns@gmail.com>
 S:	Maintained
diff --git a/drivers/iio/light/Kconfig b/drivers/iio/light/Kconfig
index 9a587d403118..6e0bd2addf9e 100644
--- a/drivers/iio/light/Kconfig
+++ b/drivers/iio/light/Kconfig
@@ -114,6 +114,18 @@ config AS73211
 	 This driver can also be built as a module.  If so, the module
 	 will be called as73211.
 
+config BH1745
+	tristate "ROHM BH1745 colour sensor"
+	depends on I2C
+	select REGMAP_I2C
+	select IIO_BUFFER
+	select IIO_TRIGGERED_BUFFER
+	help
+	  Say Y here to build support for the ROHM bh1745 colour sensor.
+
+	  To compile this driver as a module, choose M here: the module will
+	  be called bh1745.
+
 config BH1750
 	tristate "ROHM BH1750 ambient light sensor"
 	depends on I2C
diff --git a/drivers/iio/light/Makefile b/drivers/iio/light/Makefile
index a30f906e91ba..939a701a06ac 100644
--- a/drivers/iio/light/Makefile
+++ b/drivers/iio/light/Makefile
@@ -13,6 +13,7 @@ obj-$(CONFIG_APDS9300)		+= apds9300.o
 obj-$(CONFIG_APDS9306)		+= apds9306.o
 obj-$(CONFIG_APDS9960)		+= apds9960.o
 obj-$(CONFIG_AS73211)		+= as73211.o
+obj-$(CONFIG_BH1745)		+= bh1745.o
 obj-$(CONFIG_BH1750)		+= bh1750.o
 obj-$(CONFIG_BH1780)		+= bh1780.o
 obj-$(CONFIG_CM32181)		+= cm32181.o
diff --git a/drivers/iio/light/bh1745.c b/drivers/iio/light/bh1745.c
new file mode 100644
index 000000000000..7962cf1c4b52
--- /dev/null
+++ b/drivers/iio/light/bh1745.c
@@ -0,0 +1,863 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * ROHM BH1745 digital colour sensor driver
+ *
+ * Copyright (C) Mudit Sharma <muditsharma.info@gmail.com>
+ *
+ * 7-bit I2C slave addresses:
+ *  0x38 (ADDR pin low)
+ *  0x39 (ADDR pin high)
+ *
+ */
+
+#include <linux/i2c.h>
+#include <linux/mutex.h>
+#include <linux/util_macros.h>
+#include <linux/iio/events.h>
+#include <linux/regmap.h>
+
+#include <linux/iio/iio.h>
+#include <linux/iio/sysfs.h>
+#include <linux/iio/trigger.h>
+#include <linux/iio/trigger_consumer.h>
+#include <linux/iio/triggered_buffer.h>
+
+#define BH1745_MOD_NAME "bh1745"
+
+/* BH1745 config regs */
+#define BH1745_SYS_CTRL 0x40
+
+#define BH1745_MODE_CTRL_1 0x41
+#define BH1745_MODE_CTRL_2 0x42
+#define BH1745_MODE_CTRL_3 0x44
+
+#define BH1745_INTR 0x60
+#define BH1745_INTR_STATUS BIT(7)
+
+#define BH1745_PERSISTENCE 0x61
+
+#define BH1745_TH_LSB 0x62
+#define BH1745_TH_MSB 0x63
+
+#define BH1745_TL_LSB 0x64
+#define BH1745_TL_MSB 0x65
+
+#define BH1745_THRESHOLD_MAX 0xFFFF
+#define BH1745_THRESHOLD_MIN 0x0
+
+#define BH1745_MANU_ID 0X92
+
+/* BH1745 output regs */
+#define BH1745_R_LSB 0x50
+#define BH1745_R_MSB 0x51
+#define BH1745_G_LSB 0x52
+#define BH1745_G_MSB 0x53
+#define BH1745_B_LSB 0x54
+#define BH1745_B_MSB 0x55
+#define BH1745_CLR_LSB 0x56
+#define BH1745_CLR_MSB 0x57
+
+#define BH1745_SW_RESET BIT(7)
+#define BH1745_INT_RESET BIT(6)
+
+#define BH1745_MEASUREMENT_TIME_MASK GENMASK(2, 0)
+
+#define BH1745_RGBC_EN BIT(4)
+
+#define BH1745_ADC_GAIN_MASK GENMASK(1, 0)
+
+#define BH1745_INT_ENABLE BIT(0)
+#define BH1745_INT_SIGNAL_ACTIVE BIT(7)
+
+#define BH1745_INT_SIGNAL_LATCHED BIT(4)
+#define BH1745_INT_SIGNAL_LATCH_OFFSET 4
+
+#define BH1745_INT_SOURCE_MASK GENMASK(3, 2)
+#define BH1745_INT_SOURCE_OFFSET 2
+
+#define BH1745_INT_TIME_AVAILABLE "0.16 0.32 0.64 1.28 2.56 5.12"
+#define BH1745_HARDWAREGAIN_AVAILABLE "1 2 16"
+#define BH1745_INT_COLOUR_CHANNEL_AVAILABLE \
+	"0 (Red Channel) 1 (Green Channel) 2 (Blue channel) 3 (Clear channel)"
+
+static const int bh1745_int_time[][2] = {
+	{ 0, 160000 }, /* 160 ms */
+	{ 0, 320000 }, /* 320 ms */
+	{ 0, 640000 }, /* 640 ms */
+	{ 1, 280000 }, /* 1280 ms */
+	{ 2, 560000 }, /* 2560 ms */
+	{ 5, 120000 }, /* 5120 ms */
+};
+
+static const u8 bh1745_gain_factor[] = { 1, 2, 16 };
+
+enum bh1745_int_source {
+	BH1745_INT_SOURCE_RED,
+	BH1745_INT_SOURCE_GREEN,
+	BH1745_INT_SOURCE_BLUE,
+	BH1745_INT_SOURCE_CLEAR,
+};
+
+enum bh1745_gain {
+	BH1745_ADC_GAIN_1X,
+	BH1745_ADC_GAIN_2X,
+	BH1745_ADC_GAIN_16X,
+};
+
+enum bh1745_measurement_time {
+	BH1745_MEASUREMENT_TIME_160MS,
+	BH1745_MEASUREMENT_TIME_320MS,
+	BH1745_MEASUREMENT_TIME_640MS,
+	BH1745_MEASUREMENT_TIME_1280MS,
+	BH1745_MEASUREMENT_TIME_2560MS,
+	BH1745_MEASUREMENT_TIME_5120MS,
+};
+
+enum bh1745_presistence_value {
+	BH1745_PRESISTENCE_UPDATE_TOGGLE,
+	BH1745_PRESISTENCE_UPDATE_EACH_MEASUREMENT,
+	BH1745_PRESISTENCE_UPDATE_FOUR_MEASUREMENT,
+	BH1745_PRESISTENCE_UPDATE_EIGHT_MEASUREMENT,
+};
+
+struct bh1745_data {
+	struct mutex lock;
+	struct regmap *regmap;
+	struct i2c_client *client;
+	struct iio_trigger *trig;
+	u8 mode_ctrl1;
+	u8 mode_ctrl2;
+	u8 int_src;
+	u8 int_latch;
+	u8 interrupt;
+};
+
+static const struct regmap_range bh1745_volatile_ranges[] = {
+	regmap_reg_range(BH1745_MODE_CTRL_2, BH1745_MODE_CTRL_2), /* VALID */
+	regmap_reg_range(BH1745_R_LSB, BH1745_CLR_MSB), /* Data */
+	regmap_reg_range(BH1745_INTR, BH1745_INTR), /* Interrupt */
+};
+
+static const struct regmap_access_table bh1745_volatile_regs = {
+	.yes_ranges = bh1745_volatile_ranges,
+	.n_yes_ranges = ARRAY_SIZE(bh1745_volatile_ranges),
+};
+
+static const struct regmap_range bh1745_read_ranges[] = {
+	regmap_reg_range(BH1745_SYS_CTRL, BH1745_MODE_CTRL_2),
+	regmap_reg_range(BH1745_R_LSB, BH1745_CLR_MSB),
+	regmap_reg_range(BH1745_INTR, BH1745_INTR),
+	regmap_reg_range(BH1745_PERSISTENCE, BH1745_TL_MSB),
+	regmap_reg_range(BH1745_MANU_ID, BH1745_MANU_ID),
+};
+
+static const struct regmap_access_table bh1745_ro_regs = {
+	.yes_ranges = bh1745_read_ranges,
+	.n_yes_ranges = ARRAY_SIZE(bh1745_read_ranges),
+};
+
+static const struct regmap_range bh1745_writable_ranges[] = {
+	regmap_reg_range(BH1745_SYS_CTRL, BH1745_MODE_CTRL_2),
+	regmap_reg_range(BH1745_PERSISTENCE, BH1745_TL_MSB),
+};
+
+static const struct regmap_access_table bh1745_wr_regs = {
+	.yes_ranges = bh1745_writable_ranges,
+	.n_yes_ranges = ARRAY_SIZE(bh1745_writable_ranges),
+};
+
+static const struct regmap_config bh1745_regmap = {
+	.reg_bits = 8,
+	.val_bits = 8,
+	.max_register = BH1745_MANU_ID,
+	.cache_type = REGCACHE_RBTREE,
+	.volatile_table = &bh1745_volatile_regs,
+	.wr_table = &bh1745_wr_regs,
+	.rd_table = &bh1745_ro_regs,
+};
+
+static const struct iio_event_spec bh1745_event_spec[] = {
+	{
+		.type = IIO_EV_TYPE_THRESH,
+		.dir = IIO_EV_DIR_RISING,
+		.mask_shared_by_type = BIT(IIO_EV_INFO_VALUE),
+	},
+	{
+		.type = IIO_EV_TYPE_THRESH,
+		.dir = IIO_EV_DIR_FALLING,
+		.mask_shared_by_type = BIT(IIO_EV_INFO_VALUE),
+	},
+	{
+		.type = IIO_EV_TYPE_THRESH,
+		.dir = IIO_EV_DIR_EITHER,
+		.mask_shared_by_type = BIT(IIO_EV_INFO_PERIOD),
+	},
+};
+
+#define BH1745_CHANNEL(_colour, _si, _addr)                                   \
+	{                                                                     \
+		.type = IIO_INTENSITY, .modified = 1,                         \
+		.info_mask_separate = BIT(IIO_CHAN_INFO_RAW),                 \
+		.info_mask_shared_by_type = BIT(IIO_CHAN_INFO_HARDWAREGAIN) | \
+					    BIT(IIO_CHAN_INFO_INT_TIME),      \
+		.event_spec = bh1745_event_spec,                              \
+		.num_event_specs = ARRAY_SIZE(bh1745_event_spec),             \
+		.channel2 = IIO_MOD_LIGHT_##_colour, .address = _addr,        \
+		.scan_index = _si,                                            \
+		.scan_type = {                                                \
+			.sign = 'u',                                          \
+			.realbits = 16,                                       \
+			.storagebits = 16,                                    \
+			.endianness = IIO_CPU,                                \
+		},                                                            \
+	}
+
+static const struct iio_chan_spec bh1745_channels[] = {
+	BH1745_CHANNEL(RED, 0, BH1745_R_LSB),
+	BH1745_CHANNEL(GREEN, 1, BH1745_G_LSB),
+	BH1745_CHANNEL(BLUE, 2, BH1745_B_LSB),
+	BH1745_CHANNEL(CLEAR, 3, BH1745_CLR_LSB),
+	IIO_CHAN_SOFT_TIMESTAMP(4),
+};
+
+static int bh1745_write_value(struct bh1745_data *data, u8 reg, void *value,
+			      size_t len)
+{
+	int ret;
+
+	ret = regmap_bulk_write(data->regmap, reg, value, len);
+	if (ret < 0) {
+		dev_err(&data->client->dev,
+			"Failed to write to sensor. Reg: 0x%x\n", reg);
+	}
+
+	return ret;
+}
+
+static int bh1745_read_value(struct bh1745_data *data, u8 reg, void *value,
+			     size_t len)
+{
+	int ret;
+
+	ret = regmap_bulk_read(data->regmap, reg, value, len);
+	if (ret < 0) {
+		dev_err(&data->client->dev,
+			"Failed to read from sensor. Reg: 0x%x\n", reg);
+	}
+
+	return ret;
+}
+
+static ssize_t in_interrupt_source_show(struct device *dev,
+					struct device_attribute *attr,
+					char *buf)
+{
+	int ret;
+	int value;
+	struct iio_dev *indio_dev = dev_to_iio_dev(dev);
+	struct bh1745_data *data = iio_priv(indio_dev);
+
+	ret = bh1745_read_value(data, BH1745_INTR, &value, 1);
+	if (ret < 0)
+		return ret;
+
+	value &= BH1745_INT_SOURCE_MASK;
+
+	return sprintf(buf, "%d\n", value >> 2);
+}
+
+static ssize_t in_interrupt_source_store(struct device *dev,
+					 struct device_attribute *attr,
+					 const char *buf, size_t len)
+{
+	int ret;
+	u16 value;
+	struct iio_dev *indio_dev = dev_to_iio_dev(dev);
+	struct bh1745_data *data = iio_priv(indio_dev);
+
+	ret = kstrtou16(buf, 10, &value);
+	if (ret < 0)
+		return ret;
+
+	if (value > BH1745_INT_SOURCE_CLEAR) {
+		dev_err(dev,
+			"Supplied value: '%d' for interrupt source is invalid\n",
+			value);
+		return -EINVAL;
+	}
+	guard(mutex)(&data->lock);
+	data->int_src = value;
+	value = value << BH1745_INT_SOURCE_OFFSET;
+	ret = bh1745_read_value(data, BH1745_INTR, &data->interrupt, 1);
+	if (ret < 0)
+		return ret;
+
+	data->interrupt &= ~BH1745_INT_SOURCE_MASK;
+	data->interrupt |= value;
+	ret = bh1745_write_value(data, BH1745_INTR, &data->interrupt, 1);
+	if (ret < 0)
+		return ret;
+
+	return len;
+}
+
+static ssize_t in_interrupt_latch_show(struct device *dev,
+				       struct device_attribute *attr, char *buf)
+{
+	int ret;
+	int value;
+	struct iio_dev *indio_dev = dev_to_iio_dev(dev);
+	struct bh1745_data *data = iio_priv(indio_dev);
+
+	ret = bh1745_read_value(data, BH1745_INTR, &value, 1);
+	if (ret < 0)
+		return ret;
+
+	value &= BH1745_INT_SIGNAL_LATCHED;
+	if (value)
+		return sprintf(buf, "1\n");
+
+	return sprintf(buf, "0\n");
+}
+
+static ssize_t in_interrupt_latch_store(struct device *dev,
+					struct device_attribute *attr,
+					const char *buf, size_t len)
+{
+	int ret;
+	u16 value;
+	struct iio_dev *indio_dev = dev_to_iio_dev(dev);
+	struct bh1745_data *data = iio_priv(indio_dev);
+
+	ret = kstrtou16(buf, 10, &value);
+	if (ret < 0)
+		return ret;
+
+	if (value > 1) {
+		dev_err(dev, "Value out of range for latch setup. Supported values '0' or '1'\n");
+		return -EINVAL;
+	}
+
+	guard(mutex)(&data->lock);
+	data->int_latch = value;
+	ret = bh1745_read_value(data, BH1745_INTR, &data->interrupt, 1);
+	if (ret < 0)
+		return ret;
+
+	if (value == 0)
+		data->interrupt &= ~BH1745_INT_SIGNAL_LATCHED;
+	else
+		data->interrupt |= BH1745_INT_SIGNAL_LATCHED;
+
+	ret = bh1745_write_value(data, BH1745_INTR, &data->interrupt, 1);
+	if (ret < 0)
+		return ret;
+
+	return len;
+}
+
+static ssize_t hardwaregain_available_show(struct device *dev,
+					   struct device_attribute *attr,
+					   char *buf)
+{
+	return sprintf(buf, "%s\n", BH1745_HARDWAREGAIN_AVAILABLE);
+}
+
+static ssize_t interrupt_source_available_show(struct device *dev,
+					       struct device_attribute *attr,
+					       char *buf)
+{
+	return sprintf(buf, "%s\n", BH1745_INT_COLOUR_CHANNEL_AVAILABLE);
+}
+
+static IIO_DEVICE_ATTR_RW(in_interrupt_source, 0);
+static IIO_DEVICE_ATTR_RW(in_interrupt_latch, 0);
+static IIO_DEVICE_ATTR_RO(hardwaregain_available, 0);
+static IIO_DEVICE_ATTR_RO(interrupt_source_available, 0);
+static IIO_CONST_ATTR_INT_TIME_AVAIL(BH1745_INT_TIME_AVAILABLE);
+
+static struct attribute *bh1745_attrs[] = {
+	&iio_dev_attr_in_interrupt_source.dev_attr.attr,
+	&iio_dev_attr_in_interrupt_latch.dev_attr.attr,
+	&iio_dev_attr_hardwaregain_available.dev_attr.attr,
+	&iio_dev_attr_interrupt_source_available.dev_attr.attr,
+	&iio_const_attr_integration_time_available.dev_attr.attr,
+	NULL
+};
+
+static const struct attribute_group bh1745_attr_group = {
+	.attrs = bh1745_attrs,
+};
+
+static int bh1745_reset(struct bh1745_data *data)
+{
+	int ret;
+	u8 value;
+
+	ret = bh1745_read_value(data, BH1745_SYS_CTRL, &value, 1);
+	if (ret < 0)
+		return ret;
+
+	value |= (BH1745_SW_RESET | BH1745_INT_RESET);
+
+	return bh1745_write_value(data, BH1745_SYS_CTRL, &value, 1);
+}
+
+static int bh1745_power_on(struct bh1745_data *data)
+{
+	int ret;
+	u8 value;
+
+	ret = bh1745_read_value(data, BH1745_MODE_CTRL_2, &value, 1);
+	if (ret < 0)
+		return ret;
+
+	guard(mutex)(&data->lock);
+	value |= BH1745_RGBC_EN;
+	data->mode_ctrl2 = value;
+	ret = bh1745_write_value(data, BH1745_MODE_CTRL_2, &data->mode_ctrl2, 1);
+
+	return ret;
+}
+
+static int bh1745_power_off(struct bh1745_data *data)
+{
+	int ret;
+	int value;
+
+	ret = bh1745_read_value(data, BH1745_MODE_CTRL_2, &value, 1);
+	if (ret < 0)
+		return ret;
+
+	guard(mutex)(&data->lock);
+	value &= ~BH1745_RGBC_EN;
+	data->mode_ctrl2 = value;
+	ret = bh1745_write_value(data, BH1745_MODE_CTRL_2, &data->mode_ctrl2, 1);
+
+	return ret;
+}
+
+static int bh1745_set_int_time(struct bh1745_data *data, int val, int val2)
+{
+	int ret;
+
+	for (u8 i = 0; i < ARRAY_SIZE(bh1745_int_time); i++) {
+		if (val == bh1745_int_time[i][0] &&
+		    val2 == bh1745_int_time[i][1]) {
+			guard(mutex)(&data->lock);
+			data->mode_ctrl1 &= ~BH1745_MEASUREMENT_TIME_MASK;
+			data->mode_ctrl1 |= i;
+			ret = bh1745_write_value(data, BH1745_MODE_CTRL_1,
+						 &data->mode_ctrl1, 1);
+			return ret;
+		}
+	}
+
+	return -EINVAL;
+}
+
+static int bh1745_read_raw(struct iio_dev *indio_dev,
+			   struct iio_chan_spec const *chan, int *val,
+			   int *val2, long mask)
+{
+	struct bh1745_data *data = iio_priv(indio_dev);
+	int ret;
+	u16 value;
+
+	switch (mask) {
+	case IIO_CHAN_INFO_RAW: {
+		ret = iio_device_claim_direct_mode(indio_dev);
+		if (ret)
+			return ret;
+		ret = bh1745_read_value(data, chan->address, &value, 2);
+		if (ret < 0)
+			return ret;
+		iio_device_release_direct_mode(indio_dev);
+		*val = value;
+		return IIO_VAL_INT;
+	}
+
+	case IIO_CHAN_INFO_HARDWAREGAIN: {
+		guard(mutex)(&data->lock);
+		ret = bh1745_read_value(data, BH1745_MODE_CTRL_2,
+					&data->mode_ctrl2, 1);
+		if (ret < 0)
+			return ret;
+
+		value = data->mode_ctrl2 & BH1745_ADC_GAIN_MASK;
+		*val = bh1745_gain_factor[value];
+		return IIO_VAL_INT;
+	}
+
+	case IIO_CHAN_INFO_INT_TIME: {
+		guard(mutex)(&data->lock);
+		ret = bh1745_read_value(data, BH1745_MODE_CTRL_1,
+					&data->mode_ctrl1, 1);
+		if (ret < 0)
+			return ret;
+
+		value = data->mode_ctrl1 & BH1745_MEASUREMENT_TIME_MASK;
+
+		*val = bh1745_int_time[value][0];
+		*val2 = bh1745_int_time[value][1];
+
+		return IIO_VAL_INT_PLUS_MICRO;
+	}
+
+	default:
+		return -EINVAL;
+	}
+}
+
+static int bh1745_write_raw(struct iio_dev *indio_dev,
+			    struct iio_chan_spec const *chan, int val, int val2,
+			    long mask)
+{
+	struct bh1745_data *data = iio_priv(indio_dev);
+	int ret;
+
+	switch (mask) {
+	case IIO_CHAN_INFO_HARDWAREGAIN: {
+		for (u8 i = 0; i < ARRAY_SIZE(bh1745_gain_factor); i++) {
+			if (bh1745_gain_factor[i] == val) {
+				guard(mutex)(&data->lock);
+				data->mode_ctrl2 &= ~BH1745_ADC_GAIN_MASK;
+				data->mode_ctrl2 |= i;
+				ret = bh1745_write_value(data,
+							 BH1745_MODE_CTRL_2,
+							 &data->mode_ctrl2, 1);
+				return ret;
+			}
+		}
+		return -EINVAL;
+	}
+
+	case IIO_CHAN_INFO_INT_TIME: {
+		return bh1745_set_int_time(data, val, val2);
+	}
+
+	default:
+		return -EINVAL;
+	}
+}
+
+static int bh1745_read_thresh(struct iio_dev *indio_dev,
+			      const struct iio_chan_spec *chan,
+			      enum iio_event_type type,
+			      enum iio_event_direction dir,
+			      enum iio_event_info info, int *val, int *val2)
+{
+	struct bh1745_data *data = iio_priv(indio_dev);
+	int ret;
+
+	switch (info) {
+	case IIO_EV_INFO_VALUE:
+		switch (dir) {
+		case IIO_EV_DIR_RISING:
+			ret = bh1745_read_value(data, BH1745_TH_LSB, val, 2);
+			if (ret < 0)
+				return ret;
+			return IIO_VAL_INT;
+		case IIO_EV_DIR_FALLING:
+			ret = bh1745_read_value(data, BH1745_TL_LSB, val, 2);
+			if (ret < 0)
+				return ret;
+			return IIO_VAL_INT;
+		default:
+			return -EINVAL;
+		}
+		break;
+	case IIO_EV_INFO_PERIOD:
+		ret = bh1745_read_value(data, BH1745_PERSISTENCE, val, 1);
+		if (ret < 0)
+			return ret;
+		return IIO_VAL_INT;
+	default:
+		return -EINVAL;
+	}
+}
+
+static int bh1745_write_thresh(struct iio_dev *indio_dev,
+			       const struct iio_chan_spec *chan,
+			       enum iio_event_type type,
+			       enum iio_event_direction dir,
+			       enum iio_event_info info, int val, int val2)
+{
+	struct bh1745_data *data = iio_priv(indio_dev);
+	int ret;
+
+	switch (info) {
+	case IIO_EV_INFO_VALUE:
+		if (val < BH1745_THRESHOLD_MIN || val > BH1745_THRESHOLD_MAX)
+			return -EINVAL;
+		switch (dir) {
+		case IIO_EV_DIR_RISING:
+			ret = bh1745_write_value(data, BH1745_TH_LSB, &val, 2);
+			if (ret < 0)
+				return ret;
+			return IIO_VAL_INT;
+		case IIO_EV_DIR_FALLING:
+			ret = bh1745_write_value(data, BH1745_TL_LSB, &val, 2);
+			if (ret < 0)
+				return ret;
+			return IIO_VAL_INT;
+		default:
+			return -EINVAL;
+		}
+		break;
+	case IIO_EV_INFO_PERIOD:
+		if (val < BH1745_PRESISTENCE_UPDATE_TOGGLE ||
+		    val > BH1745_PRESISTENCE_UPDATE_EIGHT_MEASUREMENT)
+			return -EINVAL;
+		ret = bh1745_write_value(data, BH1745_PERSISTENCE, &val, 1);
+		if (ret < 0)
+			return ret;
+		return IIO_VAL_INT;
+	default:
+		return -EINVAL;
+	}
+}
+
+static const struct iio_info bh1745_info = {
+	.attrs = &bh1745_attr_group,
+	.read_raw = bh1745_read_raw,
+	.write_raw = bh1745_write_raw,
+	.read_event_value = bh1745_read_thresh,
+	.write_event_value = bh1745_write_thresh,
+
+};
+
+static void bh1745_remove(struct i2c_client *client)
+{
+	struct iio_dev *indio_dev = i2c_get_clientdata(client);
+	struct bh1745_data *data = iio_priv(indio_dev);
+
+	if (bh1745_power_off(data) < 0)
+		dev_err(&data->client->dev, "Failed to turn off device");
+
+	dev_info(&data->client->dev, "BH1745 driver removed\n");
+}
+
+static int bh1745_set_trigger_state(struct iio_trigger *trig, bool state)
+{
+	int ret;
+	u8 value = 0;
+	struct iio_dev *indio_dev = iio_trigger_get_drvdata(trig);
+	struct bh1745_data *data = iio_priv(indio_dev);
+
+	guard(mutex)(&data->lock);
+	if (state) {
+		ret = bh1745_read_value(data, BH1745_INTR, &value, 1);
+		if (ret < 0)
+			return ret;
+		value |= (BH1745_INT_ENABLE |
+			(data->int_latch << BH1745_INT_SIGNAL_LATCH_OFFSET) |
+			(data->int_src << BH1745_INT_SOURCE_OFFSET));
+		data->interrupt = value;
+		ret = bh1745_write_value(data, BH1745_INTR, &data->interrupt, 1);
+	} else {
+		data->interrupt = value;
+		ret = bh1745_write_value(data, BH1745_INTR, &data->interrupt, 1);
+	}
+
+	return ret;
+}
+
+static const struct iio_trigger_ops bh1745_trigger_ops = {
+	.set_trigger_state = bh1745_set_trigger_state,
+};
+
+static irqreturn_t bh1745_interrupt_handler(int interrupt, void *p)
+{
+	struct iio_dev *indio_dev = p;
+	struct bh1745_data *data = iio_priv(indio_dev);
+	int ret;
+	int value;
+
+	ret = bh1745_read_value(data, BH1745_INTR, &value, 1);
+	if (ret < 0)
+		return IRQ_NONE;
+
+	if (value & BH1745_INTR_STATUS) {
+		guard(mutex)(&data->lock);
+		iio_push_event(indio_dev,
+			       IIO_UNMOD_EVENT_CODE(IIO_INTENSITY,
+						    data->int_src,
+						    IIO_EV_TYPE_THRESH,
+						    IIO_EV_DIR_EITHER),
+			       iio_get_time_ns(indio_dev));
+
+		iio_trigger_poll_nested(data->trig);
+		return IRQ_HANDLED;
+	}
+
+	return IRQ_NONE;
+}
+
+static irqreturn_t bh1745_trigger_handler(int interrupt, void *p)
+{
+	struct iio_poll_func *pf = p;
+	struct iio_dev *indio_dev = pf->indio_dev;
+	struct bh1745_data *data = iio_priv(indio_dev);
+	struct {
+		u16 chans[4];
+		s64 timestamp __aligned(8);
+	} scan;
+	u16 value;
+	int ret;
+	int i, j = 0;
+
+	for_each_set_bit(i, indio_dev->active_scan_mask, indio_dev->masklength) {
+		ret = bh1745_read_value(data, BH1745_R_LSB + 2 * i, &value, 2);
+		if (ret < 0)
+			goto err;
+		scan.chans[j++] = value;
+	}
+
+	iio_push_to_buffers_with_timestamp(indio_dev, &scan,
+					   iio_get_time_ns(indio_dev));
+
+err:
+	iio_trigger_notify_done(indio_dev->trig);
+
+	return IRQ_HANDLED;
+}
+
+static int bh1745_setup_trigger(struct iio_dev *indio_dev)
+{
+	struct bh1745_data *data = iio_priv(indio_dev);
+	int ret;
+
+	data->trig = devm_iio_trigger_alloc(indio_dev->dev.parent,
+					    "%sdata-rdy-dev%d", indio_dev->name,
+					    iio_device_id(indio_dev));
+	if (!data->trig)
+		return -ENOMEM;
+
+	data->trig->ops = &bh1745_trigger_ops;
+	iio_trigger_set_drvdata(data->trig, indio_dev);
+
+	ret = devm_iio_trigger_register(&data->client->dev, data->trig);
+	if (ret)
+		return dev_err_probe(&data->client->dev, ret,
+				     "Trigger registration failed\n");
+
+	ret = devm_iio_triggered_buffer_setup(indio_dev->dev.parent, indio_dev,
+					      NULL, bh1745_trigger_handler,
+					      NULL);
+	if (ret)
+		return dev_err_probe(&data->client->dev, ret,
+				     "Triggered buffer setup failed\n");
+
+	ret = devm_request_threaded_irq(&data->client->dev, data->client->irq,
+					NULL, bh1745_interrupt_handler,
+					IRQF_TRIGGER_FALLING | IRQF_ONESHOT,
+					"bh1745_interrupt", indio_dev);
+	if (ret)
+		return dev_err_probe(&data->client->dev, ret,
+				     "Request for IRQ failed\n");
+
+	dev_info(&data->client->dev,
+		 "Triggered buffer and IRQ setup successfully");
+
+	return ret;
+}
+
+static int bh1745_init(struct bh1745_data *data)
+{
+	int ret;
+
+	mutex_init(&data->lock);
+	data->mode_ctrl1 = 0;
+	data->mode_ctrl2 = 0;
+	data->interrupt = 0;
+	data->int_src = BH1745_INT_SOURCE_RED;
+
+	ret = bh1745_reset(data);
+	if (ret < 0) {
+		dev_err(&data->client->dev, "Failed to reset sensor\n");
+		return ret;
+	}
+
+	ret = bh1745_power_on(data);
+	if (ret < 0) {
+		dev_err(&data->client->dev, "Failed to turn on sensor\n");
+		return ret;
+	}
+
+	return ret;
+}
+
+static int bh1745_probe(struct i2c_client *client)
+{
+	int ret;
+	struct bh1745_data *data;
+	struct iio_dev *indio_dev;
+
+	indio_dev = devm_iio_device_alloc(&client->dev, sizeof(*data));
+	if (!indio_dev)
+		return -ENOMEM;
+
+	data = iio_priv(indio_dev);
+	i2c_set_clientdata(client, indio_dev);
+	data->client = client;
+	indio_dev->info = &bh1745_info;
+	indio_dev->name = "bh1745";
+	indio_dev->channels = bh1745_channels;
+	indio_dev->modes = INDIO_DIRECT_MODE;
+	indio_dev->num_channels = ARRAY_SIZE(bh1745_channels);
+
+	data->regmap = devm_regmap_init_i2c(client, &bh1745_regmap);
+	if (IS_ERR(data->regmap))
+		return dev_err_probe(&client->dev, PTR_ERR(data->regmap),
+				     "Failed to initialize Regmap\n");
+
+	ret = bh1745_init(data);
+	if (ret < 0)
+		return ret;
+
+	if (client->irq) {
+		ret = bh1745_setup_trigger(indio_dev);
+		if (ret < 0)
+			return ret;
+	}
+
+	ret = devm_iio_device_register(&client->dev, indio_dev);
+	if (ret < 0) {
+		dev_err(&data->client->dev, "Failed to register device\n");
+		return ret;
+	}
+
+	dev_info(&data->client->dev, "BH1745 driver loaded\n");
+
+	return ret;
+}
+
+static const struct i2c_device_id bh1745_idtable[] = {
+	{ "bh1745" },
+	{},
+};
+
+static const struct of_device_id bh1745_of_match[] = {
+	{
+		.compatible = "rohm,bh1745",
+	},
+	{},
+};
+
+MODULE_DEVICE_TABLE(i2c, bh1745_idtable);
+
+static struct i2c_driver bh1745_driver = {
+	.driver = {
+		.name = "bh1745",
+		.of_match_table = bh1745_of_match,
+	},
+	.probe = bh1745_probe,
+	.remove = bh1745_remove,
+	.id_table = bh1745_idtable,
+};
+
+module_i2c_driver(bh1745_driver);
+MODULE_AUTHOR("Mudit Sharma <muditsharma.info@gmail.com>");
+MODULE_DESCRIPTION("BH1745 colour sensor driver");
+MODULE_LICENSE("GPL");
-- 
2.43.0

Re: [PATCH v4 2/2] iio: light: ROHM BH1745 colour sensor

[Jonathan Cameron]

On Thu,  6 Jun 2024 17:29:42 +0100
Mudit Sharma <muditsharma.info@gmail.com> wrote:

> Add support for BH1745, which is an I2C colour sensor with red, green,
> blue and clear channels. It has a programmable active low interrupt
> pin. Interrupt occurs when the signal from the selected interrupt
> source channel crosses set interrupt threshold high or low level.
> 
> This driver includes device attributes to configure the following:
> - Interrupt pin latch: The interrupt pin can be configured to
>   be latched (until interrupt register (0x60) is read or initialized)
>   or update after each measurement.
> - Interrupt source: The colour channel that will cause the interrupt
>   when channel will cross the set threshold high or low level.
> 
> This driver also includes device attributes to present valid
> configuration options/values for:
> - Integration time
> - Interrupt colour source
> - Hardware gain
> 
> Add myself as the maintainer for this driver in MAINTAINERS.
> 
> Signed-off-by: Mudit Sharma <muditsharma.info@gmail.com>
> Reviewed-by: Ivan Orlov <ivan.orlov0322@gmail.com>
> Reviewed-by: Javier Carrasco <javier.carrasco.cruz@gmail.com>

Hi Mudit,

Welcome to IIO.

Some comments inline.  Some apply more widely than where I've
called them out, so please look for similar cases and tidy them all
up for v5.

Thanks,

Jonathan

> diff --git a/drivers/iio/light/bh1745.c b/drivers/iio/light/bh1745.c
> new file mode 100644
> index 000000000000..7962cf1c4b52
> --- /dev/null
> +++ b/drivers/iio/light/bh1745.c
> @@ -0,0 +1,863 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * ROHM BH1745 digital colour sensor driver
> + *
> + * Copyright (C) Mudit Sharma <muditsharma.info@gmail.com>
> + *
> + * 7-bit I2C slave addresses:
> + *  0x38 (ADDR pin low)
> + *  0x39 (ADDR pin high)
> + *
This blank line seems unnecessary

> + */
> +
> +#include <linux/i2c.h>
> +#include <linux/mutex.h>
> +#include <linux/util_macros.h>
> +#include <linux/iio/events.h>
> +#include <linux/regmap.h>
> +
> +#include <linux/iio/iio.h>
> +#include <linux/iio/sysfs.h>
> +#include <linux/iio/trigger.h>
> +#include <linux/iio/trigger_consumer.h>
> +#include <linux/iio/triggered_buffer.h>
> +
> +#define BH1745_MOD_NAME "bh1745"
Drop this as it's not used and not particularly useful.

> +
> +/* BH1745 config regs */
> +#define BH1745_SYS_CTRL 0x40
> +
> +#define BH1745_MODE_CTRL_1 0x41
> +#define BH1745_MODE_CTRL_2 0x42
> +#define BH1745_MODE_CTRL_3 0x44
> +
> +#define BH1745_INTR 0x60
> +#define BH1745_INTR_STATUS BIT(7)
> +
> +#define BH1745_PERSISTENCE 0x61
> +
> +#define BH1745_TH_LSB 0x62
> +#define BH1745_TH_MSB 0x63
> +
> +#define BH1745_TL_LSB 0x64
> +#define BH1745_TL_MSB 0x65
> +
> +#define BH1745_THRESHOLD_MAX 0xFFFF
> +#define BH1745_THRESHOLD_MIN 0x0
I think these are only used in one place and are 'real' not 'magic' numbers
so I'd just put the values there.

> +
> +#define BH1745_MANU_ID 0X92

Reading the manufacturer id back and printing a dev_warn() if it
doesn't match the expected can be useful for detecting when DT hasn't
been updated for a new board design and we aren't absolutely sure the
device is compatible.

> +
> +/* BH1745 output regs */
> +#define BH1745_R_LSB 0x50
I'd spell out RED, GREEN, BLUE, CLEAR
(CLEAR in particular as I thought it meant color for a while)

> +#define BH1745_R_MSB 0x51
> +#define BH1745_G_LSB 0x52
> +#define BH1745_G_MSB 0x53
> +#define BH1745_B_LSB 0x54
> +#define BH1745_B_MSB 0x55
> +#define BH1745_CLR_LSB 0x56
> +#define BH1745_CLR_MSB 0x57
> +
> +#define BH1745_SW_RESET BIT(7)
> +#define BH1745_INT_RESET BIT(6)
> +
> +#define BH1745_MEASUREMENT_TIME_MASK GENMASK(2, 0)
> +
> +#define BH1745_RGBC_EN BIT(4)
> +
> +#define BH1745_ADC_GAIN_MASK GENMASK(1, 0)
> +
> +#define BH1745_INT_ENABLE BIT(0)
> +#define BH1745_INT_SIGNAL_ACTIVE BIT(7)
> +
> +#define BH1745_INT_SIGNAL_LATCHED BIT(4)
> +#define BH1745_INT_SIGNAL_LATCH_OFFSET 4
Use FIELD_PREP() and FIELD_GET() through out and all
you need is the masks as those macros derive offsets
from the mask.

Then you can drop the defines for offsets.

> +
> +#define BH1745_INT_SOURCE_MASK GENMASK(3, 2)
> +#define BH1745_INT_SOURCE_OFFSET 2
> +
> +#define BH1745_INT_TIME_AVAILABLE "0.16 0.32 0.64 1.28 2.56 5.12"
> +#define BH1745_HARDWAREGAIN_AVAILABLE "1 2 16"
> +#define BH1745_INT_COLOUR_CHANNEL_AVAILABLE \
> +	"0 (Red Channel) 1 (Green Channel) 2 (Blue channel) 3 (Clear channel)"
> +
> +static const int bh1745_int_time[][2] = {
> +	{ 0, 160000 }, /* 160 ms */
> +	{ 0, 320000 }, /* 320 ms */
> +	{ 0, 640000 }, /* 640 ms */
> +	{ 1, 280000 }, /* 1280 ms */
> +	{ 2, 560000 }, /* 2560 ms */
> +	{ 5, 120000 }, /* 5120 ms */
> +};
> +
> +static const u8 bh1745_gain_factor[] = { 1, 2, 16 };
> +
> +enum bh1745_int_source {
> +	BH1745_INT_SOURCE_RED,
> +	BH1745_INT_SOURCE_GREEN,
> +	BH1745_INT_SOURCE_BLUE,
> +	BH1745_INT_SOURCE_CLEAR,
> +};
> +
> +enum bh1745_gain {
> +	BH1745_ADC_GAIN_1X,
> +	BH1745_ADC_GAIN_2X,
> +	BH1745_ADC_GAIN_16X,
> +};
> +
> +enum bh1745_measurement_time {
> +	BH1745_MEASUREMENT_TIME_160MS,
> +	BH1745_MEASUREMENT_TIME_320MS,
> +	BH1745_MEASUREMENT_TIME_640MS,
> +	BH1745_MEASUREMENT_TIME_1280MS,
> +	BH1745_MEASUREMENT_TIME_2560MS,
> +	BH1745_MEASUREMENT_TIME_5120MS,
> +};
> +
> +enum bh1745_presistence_value {
> +	BH1745_PRESISTENCE_UPDATE_TOGGLE,
> +	BH1745_PRESISTENCE_UPDATE_EACH_MEASUREMENT,
> +	BH1745_PRESISTENCE_UPDATE_FOUR_MEASUREMENT,
> +	BH1745_PRESISTENCE_UPDATE_EIGHT_MEASUREMENT,
> +};
> +
> +struct bh1745_data {
> +	struct mutex lock;

What data does this lock protect?  Locks should always have a
comment on this as it is rarely that obvious.

> +	struct regmap *regmap;
> +	struct i2c_client *client;
> +	struct iio_trigger *trig;
> +	u8 mode_ctrl1;

Why do you need to cache them?  If you do, use regmap caching
to do it for you.

> +	u8 mode_ctrl2;
> +	u8 int_src;
> +	u8 int_latch;
> +	u8 interrupt;
> +};

> +static const struct iio_event_spec bh1745_event_spec[] = {
> +	{
> +		.type = IIO_EV_TYPE_THRESH,
> +		.dir = IIO_EV_DIR_RISING,
> +		.mask_shared_by_type = BIT(IIO_EV_INFO_VALUE),
> +	},
> +	{
> +		.type = IIO_EV_TYPE_THRESH,
> +		.dir = IIO_EV_DIR_FALLING,
> +		.mask_shared_by_type = BIT(IIO_EV_INFO_VALUE),
> +	},
> +	{
> +		.type = IIO_EV_TYPE_THRESH,
> +		.dir = IIO_EV_DIR_EITHER,
> +		.mask_shared_by_type = BIT(IIO_EV_INFO_PERIOD),
> +	},
> +};
> +
> +#define BH1745_CHANNEL(_colour, _si, _addr)                                   \
> +	{                                                                     \
> +		.type = IIO_INTENSITY, .modified = 1,                         \
> +		.info_mask_separate = BIT(IIO_CHAN_INFO_RAW),                 \
> +		.info_mask_shared_by_type = BIT(IIO_CHAN_INFO_HARDWAREGAIN) | \

Provide _SCALE instead of HARDWAREGAIN
As it's an intensity channel (and units are tricky for color sensors given
frequency dependence etc) all you need to do is ensure that if you halve
the _scale and measure the same light source, the computed
_RAW * _SCALE value remains constant.

> +					    BIT(IIO_CHAN_INFO_INT_TIME),      \
> +		.event_spec = bh1745_event_spec,                              \
> +		.num_event_specs = ARRAY_SIZE(bh1745_event_spec),             \
> +		.channel2 = IIO_MOD_LIGHT_##_colour, .address = _addr,        \
> +		.scan_index = _si,                                            \
> +		.scan_type = {                                                \
> +			.sign = 'u',                                          \
> +			.realbits = 16,                                       \
> +			.storagebits = 16,                                    \
> +			.endianness = IIO_CPU,                                \
> +		},                                                            \
> +	}
> +
> +static const struct iio_chan_spec bh1745_channels[] = {
> +	BH1745_CHANNEL(RED, 0, BH1745_R_LSB),
> +	BH1745_CHANNEL(GREEN, 1, BH1745_G_LSB),
> +	BH1745_CHANNEL(BLUE, 2, BH1745_B_LSB),
> +	BH1745_CHANNEL(CLEAR, 3, BH1745_CLR_LSB),
> +	IIO_CHAN_SOFT_TIMESTAMP(4),
> +};
> +
> +static int bh1745_write_value(struct bh1745_data *data, u8 reg, void *value,
> +			      size_t len)
> +{
> +	int ret;
These wrappers are of limited value. I'd get rid of them and make
direct regmap calls.

Regmap has a lot of debug print handling etc so no need to print
your own messages unless they convey more meaning such as 'what'
the write that failed was about.  That is better known at callers.

In many cases you are only writing one register so you can use
the simpler regmap_write()


> +
> +	ret = regmap_bulk_write(data->regmap, reg, value, len);
> +	if (ret < 0) {
regmap never returns > 0
so it is a good idea to just check
	if (ret)
through out - that avoids the handling being different for earlier
calls
	if (ret < 0)
		return ret;
which just leaves postive values in place.
and final calls
	return XXX;
which return the positive.

If you make it such that ret is never positive and so just use
if (ret)
everything is much more consistent and easier to read.

> +		dev_err(&data->client->dev,
> +			"Failed to write to sensor. Reg: 0x%x\n", reg);
> +	}
> +
> +	return ret;
> +}
> +
> +static int bh1745_read_value(struct bh1745_data *data, u8 reg, void *value,
> +			     size_t len)
> +{
> +	int ret;
> +
As for write. I'm not convinced this adds anything beyond making bulk
calls for single accesses which is not a good thing to do.

> +	ret = regmap_bulk_read(data->regmap, reg, value, len);
> +	if (ret < 0) {
> +		dev_err(&data->client->dev,
> +			"Failed to read from sensor. Reg: 0x%x\n", reg);
> +	}
> +
> +	return ret;
> +}
> +
> +static ssize_t in_interrupt_source_show(struct device *dev,
> +					struct device_attribute *attr,
> +					char *buf)
> +{
> +	int ret;
> +	int value;
> +	struct iio_dev *indio_dev = dev_to_iio_dev(dev);
> +	struct bh1745_data *data = iio_priv(indio_dev);
> +
> +	ret = bh1745_read_value(data, BH1745_INTR, &value, 1);
> +	if (ret < 0)
> +		return ret;
> +
> +	value &= BH1745_INT_SOURCE_MASK;
> +
> +	return sprintf(buf, "%d\n", value >> 2);
> +}
> +
> +static ssize_t in_interrupt_source_store(struct device *dev,
> +					 struct device_attribute *attr,
> +					 const char *buf, size_t len)
As mentioned elsewhere I don't see why this can't be done with 4
event enables and a policy of last one set wins.  
It's common for devices to support a limited number of events
at a time so userspace needs to be aware of that and check to
see whether events it previously enable are still enabled after
requesting a new one.


> +{
> +	int ret;
> +	u16 value;
> +	struct iio_dev *indio_dev = dev_to_iio_dev(dev);
> +	struct bh1745_data *data = iio_priv(indio_dev);
> +
> +	ret = kstrtou16(buf, 10, &value);
> +	if (ret < 0)
> +		return ret;
> +
> +	if (value > BH1745_INT_SOURCE_CLEAR) {
> +		dev_err(dev,
> +			"Supplied value: '%d' for interrupt source is invalid\n",
> +			value);
> +		return -EINVAL;
> +	}
> +	guard(mutex)(&data->lock);
> +	data->int_src = value;
> +	value = value << BH1745_INT_SOURCE_OFFSET;
> +	ret = bh1745_read_value(data, BH1745_INTR, &data->interrupt, 1);
> +	if (ret < 0)
> +		return ret;
> +
> +	data->interrupt &= ~BH1745_INT_SOURCE_MASK;
> +	data->interrupt |= value;
> +	ret = bh1745_write_value(data, BH1745_INTR, &data->interrupt, 1);
> +	if (ret < 0)
> +		return ret;
> +
> +	return len;
> +}
> +
> +static ssize_t in_interrupt_latch_show(struct device *dev,
> +				       struct device_attribute *attr, char *buf)
> +{
> +	int ret;
> +	int value;
> +	struct iio_dev *indio_dev = dev_to_iio_dev(dev);
> +	struct bh1745_data *data = iio_priv(indio_dev);
> +
> +	ret = bh1745_read_value(data, BH1745_INTR, &value, 1);
> +	if (ret < 0)
> +		return ret;
> +
> +	value &= BH1745_INT_SIGNAL_LATCHED;
> +	if (value)
> +		return sprintf(buf, "1\n");
> +
> +	return sprintf(buf, "0\n");

Always enable latch.  We don't want an interrupt storm so
latch on and level interrupt is probably the best option for a linux driver.
Note level not edge unless the driver guarantees to drop the interrupt
line for substantial time after the interrupt is cleared.

Providing userspace control for this is not likely to be very useful.

> +}
> +
> +static ssize_t in_interrupt_latch_store(struct device *dev,
> +					struct device_attribute *attr,
> +					const char *buf, size_t len)
> +{
> +	int ret;
> +	u16 value;
> +	struct iio_dev *indio_dev = dev_to_iio_dev(dev);
> +	struct bh1745_data *data = iio_priv(indio_dev);
> +
> +	ret = kstrtou16(buf, 10, &value);
> +	if (ret < 0)
> +		return ret;
> +
> +	if (value > 1) {
> +		dev_err(dev, "Value out of range for latch setup. Supported values '0' or '1'\n");
> +		return -EINVAL;
> +	}
> +
> +	guard(mutex)(&data->lock);
> +	data->int_latch = value;
> +	ret = bh1745_read_value(data, BH1745_INTR, &data->interrupt, 1);
> +	if (ret < 0)
> +		return ret;
> +
> +	if (value == 0)
> +		data->interrupt &= ~BH1745_INT_SIGNAL_LATCHED;
> +	else
> +		data->interrupt |= BH1745_INT_SIGNAL_LATCHED;
> +
> +	ret = bh1745_write_value(data, BH1745_INTR, &data->interrupt, 1);
> +	if (ret < 0)
> +		return ret;
> +
> +	return len;
> +}
> +
> +static ssize_t hardwaregain_available_show(struct device *dev,
> +					   struct device_attribute *attr,
> +					   char *buf)
> +{
> +	return sprintf(buf, "%s\n", BH1745_HARDWAREGAIN_AVAILABLE);
> +}
> +
> +static ssize_t interrupt_source_available_show(struct device *dev,
> +					       struct device_attribute *attr,
> +					       char *buf)
> +{
> +	return sprintf(buf, "%s\n", BH1745_INT_COLOUR_CHANNEL_AVAILABLE);
> +}
> +
> +static IIO_DEVICE_ATTR_RW(in_interrupt_source, 0);

Why don't he normal event enables work for this?

> +static IIO_DEVICE_ATTR_RW(in_interrupt_latch, 0);
> +static IIO_DEVICE_ATTR_RO(hardwaregain_available, 0);

Hardware gain is actually rarely seen.  It is intended for cases
where the gain is independent from the actual type of signal being
measured. e.g. light gain in a time of flight sensor which is measuring
distance.

Mostly we map controls of sensor gain to either
scale, or calibscale (if they are meant to correct for variation
between sensors coming off the production line).

> +static IIO_DEVICE_ATTR_RO(interrupt_source_available, 0);
> +static IIO_CONST_ATTR_INT_TIME_AVAIL(BH1745_INT_TIME_AVAILABLE);
> +
> +static struct attribute *bh1745_attrs[] = {
> +	&iio_dev_attr_in_interrupt_source.dev_attr.attr,
> +	&iio_dev_attr_in_interrupt_latch.dev_attr.attr,

Custom ABI needs documentation. Note that you need a very strong justification
for it.  So first step is to consider if any existing ABI is appropriate.
See Documentation/ABI/testing/sysfs-bus-iio*


> +	&iio_dev_attr_hardwaregain_available.dev_attr.attr,
> +	&iio_dev_attr_interrupt_source_available.dev_attr.attr,
> +	&iio_const_attr_integration_time_available.dev_attr.attr,
Use the get_avail() callback for the standard ABI.

> +	NULL
> +};
> +
> +static const struct attribute_group bh1745_attr_group = {
> +	.attrs = bh1745_attrs,
> +};
> +
> +static int bh1745_reset(struct bh1745_data *data)
> +{
> +	int ret;
> +	u8 value;
> +
> +	ret = bh1745_read_value(data, BH1745_SYS_CTRL, &value, 1);
> +	if (ret < 0)
> +		return ret;
> +
> +	value |= (BH1745_SW_RESET | BH1745_INT_RESET);
> +
> +	return bh1745_write_value(data, BH1745_SYS_CTRL, &value, 1);
This is an example of the different handling for ret depending on whether
it is the last call or not that I mention above.

	if (ret)
for all error checks and it becomes consistent.

That way you can do if (ret) to check if hb1745_reset() has failed at call
sites etc.


> +}
> +
> +static int bh1745_power_on(struct bh1745_data *data)
> +{
> +	int ret;
> +	u8 value;
> +
> +	ret = bh1745_read_value(data, BH1745_MODE_CTRL_2, &value, 1);
> +	if (ret < 0)
> +		return ret;
> +
> +	guard(mutex)(&data->lock);
> +	value |= BH1745_RGBC_EN;
> +	data->mode_ctrl2 = value;
> +	ret = bh1745_write_value(data, BH1745_MODE_CTRL_2, &data->mode_ctrl2, 1);
> +
> +	return ret;

	return bh1745_write...

> +}
> +
> +static int bh1745_power_off(struct bh1745_data *data)
> +{
> +	int ret;
> +	int value;
> +
> +	ret = bh1745_read_value(data, BH1745_MODE_CTRL_2, &value, 1);
> +	if (ret < 0)
> +		return ret;
> +
> +	guard(mutex)(&data->lock);
> +	value &= ~BH1745_RGBC_EN;
> +	data->mode_ctrl2 = value;
> +	ret = bh1745_write_value(data, BH1745_MODE_CTRL_2, &data->mode_ctrl2, 1);
> +
> +	return ret;

same as below. Don't assign to a local variable just to return it on the
next line.

> +}
> +
> +static int bh1745_set_int_time(struct bh1745_data *data, int val, int val2)
> +{
> +	int ret;
> +
> +	for (u8 i = 0; i < ARRAY_SIZE(bh1745_int_time); i++) {
> +		if (val == bh1745_int_time[i][0] &&
> +		    val2 == bh1745_int_time[i][1]) {
> +			guard(mutex)(&data->lock);
> +			data->mode_ctrl1 &= ~BH1745_MEASUREMENT_TIME_MASK;
> +			data->mode_ctrl1 |= i;
> +			ret = bh1745_write_value(data, BH1745_MODE_CTRL_1,
> +						 &data->mode_ctrl1, 1);
> +			return ret;
			return bh17..
> +		}
> +	}
> +
> +	return -EINVAL;
> +}
> +
> +static int bh1745_read_raw(struct iio_dev *indio_dev,
> +			   struct iio_chan_spec const *chan, int *val,
> +			   int *val2, long mask)
> +{
> +	struct bh1745_data *data = iio_priv(indio_dev);
> +	int ret;
> +	u16 value;
> +
> +	switch (mask) {
> +	case IIO_CHAN_INFO_RAW: {
> +		ret = iio_device_claim_direct_mode(indio_dev);
> +		if (ret)
> +			return ret;
> +		ret = bh1745_read_value(data, chan->address, &value, 2);
> +		if (ret < 0)
> +			return ret;
Exited with the device held in direct mode for ever.

Use iio_device_claim_direct_mode_scoped() that deals with this
for you.

> +		iio_device_release_direct_mode(indio_dev);
> +		*val = value;
> +		return IIO_VAL_INT;
> +	}
> +
> +	case IIO_CHAN_INFO_HARDWAREGAIN: {
> +		guard(mutex)(&data->lock);
> +		ret = bh1745_read_value(data, BH1745_MODE_CTRL_2,
> +					&data->mode_ctrl2, 1);
> +		if (ret < 0)
> +			return ret;
> +
> +		value = data->mode_ctrl2 & BH1745_ADC_GAIN_MASK;

FIELD_GET() as then we don't need to look to see if GAIN_MASK starts
at bit 0.

> +		*val = bh1745_gain_factor[value];
> +		return IIO_VAL_INT;
> +	}
> +
> +	case IIO_CHAN_INFO_INT_TIME: {
> +		guard(mutex)(&data->lock);
> +		ret = bh1745_read_value(data, BH1745_MODE_CTRL_1,
> +					&data->mode_ctrl1, 1);
> +		if (ret < 0)
> +			return ret;
> +
> +		value = data->mode_ctrl1 & BH1745_MEASUREMENT_TIME_MASK;
FIELD_GET() for this and all similar masking or masking and shifting operations.

> +
> +		*val = bh1745_int_time[value][0];
> +		*val2 = bh1745_int_time[value][1];
> +
> +		return IIO_VAL_INT_PLUS_MICRO;
> +	}
> +
> +	default:
> +		return -EINVAL;
> +	}
> +}
> +
> +static int bh1745_write_raw(struct iio_dev *indio_dev,
> +			    struct iio_chan_spec const *chan, int val, int val2,
> +			    long mask)
> +{
> +	struct bh1745_data *data = iio_priv(indio_dev);
> +	int ret;
> +
> +	switch (mask) {
> +	case IIO_CHAN_INFO_HARDWAREGAIN: {
> +		for (u8 i = 0; i < ARRAY_SIZE(bh1745_gain_factor); i++) {
> +			if (bh1745_gain_factor[i] == val) {
Flip logic here.
			if (bh1745_gain_factor[i] != val)
				continue;

as reduces indent on the following code whilst remaining almost as easy
to read.

> +				guard(mutex)(&data->lock);
> +				data->mode_ctrl2 &= ~BH1745_ADC_GAIN_MASK;
> +				data->mode_ctrl2 |= i;
> +				ret = bh1745_write_value(data,
> +							 BH1745_MODE_CTRL_2,
> +							 &data->mode_ctrl2, 1);
> +				return ret;

				return bh17...
Then ret unused so drop that as well.
> +			}
> +		}
> +		return -EINVAL;
> +	}
> +
> +	case IIO_CHAN_INFO_INT_TIME: {
> +		return bh1745_set_int_time(data, val, val2);
> +	}
> +
> +	default:
> +		return -EINVAL;
> +	}
> +}
> +
> +static int bh1745_read_thresh(struct iio_dev *indio_dev,
> +			      const struct iio_chan_spec *chan,
> +			      enum iio_event_type type,
> +			      enum iio_event_direction dir,
> +			      enum iio_event_info info, int *val, int *val2)
> +{
> +	struct bh1745_data *data = iio_priv(indio_dev);
> +	int ret;
> +
> +	switch (info) {
> +	case IIO_EV_INFO_VALUE:
> +		switch (dir) {
> +		case IIO_EV_DIR_RISING:
> +			ret = bh1745_read_value(data, BH1745_TH_LSB, val, 2);
> +			if (ret < 0)
> +				return ret;
> +			return IIO_VAL_INT;
> +		case IIO_EV_DIR_FALLING:
> +			ret = bh1745_read_value(data, BH1745_TL_LSB, val, 2);
> +			if (ret < 0)
> +				return ret;
> +			return IIO_VAL_INT;
> +		default:
> +			return -EINVAL;
> +		}
> +		break;

Unreachable. Drop this break line.

> +	case IIO_EV_INFO_PERIOD:
> +		ret = bh1745_read_value(data, BH1745_PERSISTENCE, val, 1);
> +		if (ret < 0)
> +			return ret;
> +		return IIO_VAL_INT;
> +	default:
> +		return -EINVAL;
> +	}
> +}
> +
> +static int bh1745_write_thresh(struct iio_dev *indio_dev,
> +			       const struct iio_chan_spec *chan,
> +			       enum iio_event_type type,
> +			       enum iio_event_direction dir,
> +			       enum iio_event_info info, int val, int val2)
> +{
> +	struct bh1745_data *data = iio_priv(indio_dev);
> +	int ret;
> +
> +	switch (info) {
> +	case IIO_EV_INFO_VALUE:
> +		if (val < BH1745_THRESHOLD_MIN || val > BH1745_THRESHOLD_MAX)
> +			return -EINVAL;
> +		switch (dir) {
> +		case IIO_EV_DIR_RISING:
> +			ret = bh1745_write_value(data, BH1745_TH_LSB, &val, 2);
> +			if (ret < 0)
> +				return ret;
> +			return IIO_VAL_INT;
> +		case IIO_EV_DIR_FALLING:
> +			ret = bh1745_write_value(data, BH1745_TL_LSB, &val, 2);
> +			if (ret < 0)
> +				return ret;
> +			return IIO_VAL_INT;
> +		default:
> +			return -EINVAL;
> +		}
> +		break;

Unreachable so drop this break line

> +	case IIO_EV_INFO_PERIOD:
> +		if (val < BH1745_PRESISTENCE_UPDATE_TOGGLE ||
> +		    val > BH1745_PRESISTENCE_UPDATE_EIGHT_MEASUREMENT)
> +			return -EINVAL;
> +		ret = bh1745_write_value(data, BH1745_PERSISTENCE, &val, 1);
> +		if (ret < 0)
> +			return ret;
> +		return IIO_VAL_INT;
> +	default:
> +		return -EINVAL;
> +	}
> +}
> +
> +static const struct iio_info bh1745_info = {
> +	.attrs = &bh1745_attr_group,
> +	.read_raw = bh1745_read_raw,
> +	.write_raw = bh1745_write_raw,
> +	.read_event_value = bh1745_read_thresh,
> +	.write_event_value = bh1745_write_thresh,
> +

Drop this blank line.

> +};
> +
> +static void bh1745_remove(struct i2c_client *client)
Remove almost always comes after probe() as that's where readers
of drivers expect it to be.
> +{
> +	struct iio_dev *indio_dev = i2c_get_clientdata(client);
> +	struct bh1745_data *data = iio_priv(indio_dev);
> +
> +	if (bh1745_power_off(data) < 0)

This suggests you are mixing devm_ handling with manual handling.
I'm fairly sure that in this case you turn the device off before the userspace
interface is removed via devm_ managed cleanup.  That makes little sense.

Use a custom callback and devm_add_action_or_reset()

Also note that you don't turn the power off in error cases in probe().
Use a devm callback will fix that as well.

> +		dev_err(&data->client->dev, "Failed to turn off device");
> +
> +	dev_info(&data->client->dev, "BH1745 driver removed\n");

Drop this.

> +}
> +
> +static int bh1745_set_trigger_state(struct iio_trigger *trig, bool state)
> +{
> +	int ret;
> +	u8 value = 0;
> +	struct iio_dev *indio_dev = iio_trigger_get_drvdata(trig);
> +	struct bh1745_data *data = iio_priv(indio_dev);
> +
> +	guard(mutex)(&data->lock);
> +	if (state) {
> +		ret = bh1745_read_value(data, BH1745_INTR, &value, 1);

Seems like you should probably mask out the int_latch and int_src
before writing those fields.  Whilst you may guarantee they are always
cleared (I think that happens below) it is more readable to clear them
explicitly here as well.


> +		if (ret < 0)
> +			return ret;
> +		value |= (BH1745_INT_ENABLE |

outer brackets not needed.

> +			(data->int_latch << BH1745_INT_SIGNAL_LATCH_OFFSET) |
> +			(data->int_src << BH1745_INT_SOURCE_OFFSET));

FIELD_PREP() for each of those,

> +		data->interrupt = value;
> +		ret = bh1745_write_value(data, BH1745_INTR, &data->interrupt, 1);
		return bh17..

No need to make reader look to see if anything else happens below.


> +	} else {
> +		data->interrupt = value;
> +		ret = bh1745_write_value(data, BH1745_INTR, &data->interrupt, 1);
		return
> +	}
> +
> +	return ret;
> +}

> +static irqreturn_t bh1745_trigger_handler(int interrupt, void *p)
> +{
> +	struct iio_poll_func *pf = p;
> +	struct iio_dev *indio_dev = pf->indio_dev;
> +	struct bh1745_data *data = iio_priv(indio_dev);
> +	struct {
> +		u16 chans[4];
> +		s64 timestamp __aligned(8);
> +	} scan;
> +	u16 value;
> +	int ret;
> +	int i, j = 0;
> +
> +	for_each_set_bit(i, indio_dev->active_scan_mask, indio_dev->masklength) {
> +		ret = bh1745_read_value(data, BH1745_R_LSB + 2 * i, &value, 2);
> +		if (ret < 0)
> +			goto err;
> +		scan.chans[j++] = value;
> +	}
> +
> +	iio_push_to_buffers_with_timestamp(indio_dev, &scan,
> +					   iio_get_time_ns(indio_dev));
> +
> +err:
> +	iio_trigger_notify_done(indio_dev->trig);
> +
> +	return IRQ_HANDLED;
> +}
> +
> +static int bh1745_setup_trigger(struct iio_dev *indio_dev)
add a struct device *dev or parent.
> +{
> +	struct bh1745_data *data = iio_priv(indio_dev);
> +	int ret;
> +
> +	data->trig = devm_iio_trigger_alloc(indio_dev->dev.parent,
I'd pass this the parent in separately, as an additional parameter.

What you have here is correct, but relies a little too much on an internal
detail of the device model and how this IIO device is registered.

> +					    "%sdata-rdy-dev%d", indio_dev->name,
> +					    iio_device_id(indio_dev));
> +	if (!data->trig)
> +		return -ENOMEM;
> +
> +	data->trig->ops = &bh1745_trigger_ops;
> +	iio_trigger_set_drvdata(data->trig, indio_dev);
> +
> +	ret = devm_iio_trigger_register(&data->client->dev, data->trig);
> +	if (ret)
> +		return dev_err_probe(&data->client->dev, ret,

You've mixed and matched how to get this parent. Pick a consistent
path. I'd just pass it in to this function from probe()


> +				     "Trigger registration failed\n");
> +
> +	ret = devm_iio_triggered_buffer_setup(indio_dev->dev.parent, indio_dev,
> +					      NULL, bh1745_trigger_handler,
> +					      NULL);
Why is the buffer support dependent on an IRQ?

If it it is I'd expect to see validate callbacks to ensure no other trigger
is used.  In most cases a sysfs or hrtimer trigger can be used but some
care is needed in the driver.  Sometimes the coupling is tighter and
we only allow a device to use it's own trigger.

> +	if (ret)
> +		return dev_err_probe(&data->client->dev, ret,
> +				     "Triggered buffer setup failed\n");
> +
> +	ret = devm_request_threaded_irq(&data->client->dev, data->client->irq,
> +					NULL, bh1745_interrupt_handler,
> +					IRQF_TRIGGER_FALLING | IRQF_ONESHOT,
> +					"bh1745_interrupt", indio_dev);
> +	if (ret)
> +		return dev_err_probe(&data->client->dev, ret,
> +				     "Request for IRQ failed\n");
> +
> +	dev_info(&data->client->dev,
> +		 "Triggered buffer and IRQ setup successfully");
To noisy. dev_dbg() at most. Probably no print at all as easy to tell
if this worked from sysfs
> +
> +	return ret;
> +}
> +
> +static int bh1745_init(struct bh1745_data *data)
> +{
	struct device *dev = &data->client->data;
> +	int ret;
> +
> +	mutex_init(&data->lock);
> +	data->mode_ctrl1 = 0;
> +	data->mode_ctrl2 = 0;
> +	data->interrupt = 0;
> +	data->int_src = BH1745_INT_SOURCE_RED;
> +
> +	ret = bh1745_reset(data);
> +	if (ret < 0) {
> +		dev_err(&data->client->dev, "Failed to reset sensor\n");
only called from probe I think so 
	if (ret)
		return dev_err_probe(dev,
> +		return ret;
> +	}
> +
> +	ret = bh1745_power_on(data);
The unwind of this is done manually. It should not (see above)
so after checking if this succeeded, call
devm_add_action_or_reset() with a suitable callback that will turn
the power off again.
> +	if (ret < 0) {
	if (ret)
		return dev_err_probe().

	return 0;

 We know this is the only value, so make it explicit as that helps the reader
understand which is the good path and what can be returned etc.
 
> +		dev_err(&data->client->dev, "Failed to turn on sensor\n");
> +		return ret;
> +	}
> +
> +	return ret;

> +}
> +
> +static int bh1745_probe(struct i2c_client *client)
> +{
> +	int ret;
> +	struct bh1745_data *data;
> +	struct iio_dev *indio_dev;

struct device *dev = &client->dev;
as you use it several times and will shorten some long lines

> +
> +	indio_dev = devm_iio_device_alloc(&client->dev, sizeof(*data));
> +	if (!indio_dev)
> +		return -ENOMEM;
> +
> +	data = iio_priv(indio_dev);
> +	i2c_set_clientdata(client, indio_dev);

Is this ever used?  If not don't set it.

> +	data->client = client;
> +	indio_dev->info = &bh1745_info;
> +	indio_dev->name = "bh1745";
> +	indio_dev->channels = bh1745_channels;
> +	indio_dev->modes = INDIO_DIRECT_MODE;
> +	indio_dev->num_channels = ARRAY_SIZE(bh1745_channels);
> +
> +	data->regmap = devm_regmap_init_i2c(client, &bh1745_regmap);
> +	if (IS_ERR(data->regmap))
> +		return dev_err_probe(&client->dev, PTR_ERR(data->regmap),
> +				     "Failed to initialize Regmap\n");
> +
> +	ret = bh1745_init(data);
> +	if (ret < 0)
> +		return ret;
> +
> +	if (client->irq) {
> +		ret = bh1745_setup_trigger(indio_dev);
> +		if (ret < 0)
> +			return ret;
> +	}
> +
> +	ret = devm_iio_device_register(&client->dev, indio_dev);
> +	if (ret < 0) {
> +		dev_err(&data->client->dev, "Failed to register device\n");
you had client available, so should have used that.
However add a local dev variable for these.
> +		return ret;
		return dev_err_probe(dev, "Failed to register device\n");
and drop the brackets.

> +	}
> +
> +	dev_info(&data->client->dev, "BH1745 driver loaded\n");
Unnecessary noise. There are lots of other ways to find that out.
So remove this print.

> +
> +	return ret;
> +}
> +
> +static const struct i2c_device_id bh1745_idtable[] = {
> +	{ "bh1745" },
> +	{},
> +};
> +
> +static const struct of_device_id bh1745_of_match[] = {
> +	{
> +		.compatible = "rohm,bh1745",
> +	},
> +	{},

No comma on trailing 'NULL' entries as we must never
add anything after them.

> +};
This needs appropriate MODULE_DEVICE_TABLE() as well

> +
> +MODULE_DEVICE_TABLE(i2c, bh1745_idtable);
Move this to immediately after the idtable (no blank line)
> +
> +static struct i2c_driver bh1745_driver = {
> +	.driver = {
> +		.name = "bh1745",
> +		.of_match_table = bh1745_of_match,
> +	},
> +	.probe = bh1745_probe,
> +	.remove = bh1745_remove,
> +	.id_table = bh1745_idtable,
> +};
> +
> +module_i2c_driver(bh1745_driver);
> +MODULE_AUTHOR("Mudit Sharma <muditsharma.info@gmail.com>");
> +MODULE_DESCRIPTION("BH1745 colour sensor driver");
> +MODULE_LICENSE("GPL");

Re: [PATCH v4 2/2] iio: light: ROHM BH1745 colour sensor

[Matti Vaittinen]

la 8. kesäk. 2024 klo 19.22 Jonathan Cameron (jic23@kernel.org) kirjoitti:
>
> On Thu,  6 Jun 2024 17:29:42 +0100
> Mudit Sharma <muditsharma.info@gmail.com> wrote:
>
> > Add support for BH1745, which is an I2C colour sensor with red, green,
> > blue and clear channels. It has a programmable active low interrupt
> > pin. Interrupt occurs when the signal from the selected interrupt
> > source channel crosses set interrupt threshold high or low level.
> >
> > This driver includes device attributes to configure the following:
> > - Interrupt pin latch: The interrupt pin can be configured to
> >   be latched (until interrupt register (0x60) is read or initialized)
> >   or update after each measurement.
> > - Interrupt source: The colour channel that will cause the interrupt
> >   when channel will cross the set threshold high or low level.
> >
> > This driver also includes device attributes to present valid
> > configuration options/values for:
> > - Integration time
> > - Interrupt colour source
> > - Hardware gain
> >

> > +
> > +#define BH1745_CHANNEL(_colour, _si, _addr)                                   \
> > +     {                                                                     \
> > +             .type = IIO_INTENSITY, .modified = 1,                         \
> > +             .info_mask_separate = BIT(IIO_CHAN_INFO_RAW),                 \
> > +             .info_mask_shared_by_type = BIT(IIO_CHAN_INFO_HARDWAREGAIN) | \
>
> Provide _SCALE instead of HARDWAREGAIN
> As it's an intensity channel (and units are tricky for color sensors given
> frequency dependence etc) all you need to do is ensure that if you halve
> the _scale and measure the same light source, the computed
> _RAW * _SCALE value remains constant.

...Which is likely to cause also the integration time setting to
impact the SCALE.

You may or may not want to see the GTS-helpers
(drivers/iio/industrialio-gts-helper.c) - which have their own tricky
corners. I think Jonathan once suggested to me to keep the
HARDWAREGAIN as a read-only attribute to ease seeing what is going on.
For the last couple of days I've been reworking the BU27034 driver to
work with the new sensor variant - and I can definitely see the value
of the read-only HARDWAREGAIN when we have per channel gain settings +
integration time setting which all contribute to the scale...


Matti Vaittinen
Linux kernel developer at ROHM Semiconductors
Oulu Finland

~~ When things go utterly wrong vim users can always type :help! ~~

Discuss - Estimate - Plan - Report and finally accomplish this:
void do_work(int time) __attribute__ ((const));

Re: [PATCH v4 2/2] iio: light: ROHM BH1745 colour sensor

[Jonathan Cameron]

On Mon, 10 Jun 2024 08:58:44 +0300
Matti Vaittinen <mazziesaccount@gmail.com> wrote:

> la 8. kesäk. 2024 klo 19.22 Jonathan Cameron (jic23@kernel.org) kirjoitti:
> >
> > On Thu,  6 Jun 2024 17:29:42 +0100
> > Mudit Sharma <muditsharma.info@gmail.com> wrote:
> >  
> > > Add support for BH1745, which is an I2C colour sensor with red, green,
> > > blue and clear channels. It has a programmable active low interrupt
> > > pin. Interrupt occurs when the signal from the selected interrupt
> > > source channel crosses set interrupt threshold high or low level.
> > >
> > > This driver includes device attributes to configure the following:
> > > - Interrupt pin latch: The interrupt pin can be configured to
> > >   be latched (until interrupt register (0x60) is read or initialized)
> > >   or update after each measurement.
> > > - Interrupt source: The colour channel that will cause the interrupt
> > >   when channel will cross the set threshold high or low level.
> > >
> > > This driver also includes device attributes to present valid
> > > configuration options/values for:
> > > - Integration time
> > > - Interrupt colour source
> > > - Hardware gain
> > >  
> 
> > > +
> > > +#define BH1745_CHANNEL(_colour, _si, _addr)                                   \
> > > +     {                                                                     \
> > > +             .type = IIO_INTENSITY, .modified = 1,                         \
> > > +             .info_mask_separate = BIT(IIO_CHAN_INFO_RAW),                 \
> > > +             .info_mask_shared_by_type = BIT(IIO_CHAN_INFO_HARDWAREGAIN) | \  
> >
> > Provide _SCALE instead of HARDWAREGAIN
> > As it's an intensity channel (and units are tricky for color sensors given
> > frequency dependence etc) all you need to do is ensure that if you halve
> > the _scale and measure the same light source, the computed
> > _RAW * _SCALE value remains constant.  
> 
> ...Which is likely to cause also the integration time setting to
> impact the SCALE.
> 
> You may or may not want to see the GTS-helpers
> (drivers/iio/industrialio-gts-helper.c) - which have their own tricky
> corners. I think Jonathan once suggested to me to keep the
> HARDWAREGAIN as a read-only attribute to ease seeing what is going on.
> For the last couple of days I've been reworking the BU27034 driver to
> work with the new sensor variant - and I can definitely see the value
> of the read-only HARDWAREGAIN when we have per channel gain settings +
> integration time setting which all contribute to the scale...

I'm wondering if that was good advice, but it's definitely better
than letting userspace control the gain and integration time separately
as there is no sensible way to know how to control that beyond -
it's a bit dark and I forgot I can change the integration time,
crank up the gain!

> 
> 
> Matti Vaittinen
> Linux kernel developer at ROHM Semiconductors
> Oulu Finland
> 
> ~~ When things go utterly wrong vim users can always type :help! ~~
> 
> Discuss - Estimate - Plan - Report and finally accomplish this:
> void do_work(int time) __attribute__ ((const));
> 

Re: [PATCH v4 2/2] iio: light: ROHM BH1745 colour sensor

[Matti Vaittinen]

On 6/11/24 20:14, Jonathan Cameron wrote:
> On Mon, 10 Jun 2024 08:58:44 +0300
> Matti Vaittinen <mazziesaccount@gmail.com> wrote:
> 
>> la 8. kesäk. 2024 klo 19.22 Jonathan Cameron (jic23@kernel.org) kirjoitti:
>>>
>>> On Thu,  6 Jun 2024 17:29:42 +0100
>>> Mudit Sharma <muditsharma.info@gmail.com> wrote:
>>>   
>>>> Add support for BH1745, which is an I2C colour sensor with red, green,
>>>> blue and clear channels. It has a programmable active low interrupt
>>>> pin. Interrupt occurs when the signal from the selected interrupt
>>>> source channel crosses set interrupt threshold high or low level.
>>>>
>>>> This driver includes device attributes to configure the following:
>>>> - Interrupt pin latch: The interrupt pin can be configured to
>>>>    be latched (until interrupt register (0x60) is read or initialized)
>>>>    or update after each measurement.
>>>> - Interrupt source: The colour channel that will cause the interrupt
>>>>    when channel will cross the set threshold high or low level.
>>>>
>>>> This driver also includes device attributes to present valid
>>>> configuration options/values for:
>>>> - Integration time
>>>> - Interrupt colour source
>>>> - Hardware gain
>>>>   
>>
>>>> +
>>>> +#define BH1745_CHANNEL(_colour, _si, _addr)                                   \
>>>> +     {                                                                     \
>>>> +             .type = IIO_INTENSITY, .modified = 1,                         \
>>>> +             .info_mask_separate = BIT(IIO_CHAN_INFO_RAW),                 \
>>>> +             .info_mask_shared_by_type = BIT(IIO_CHAN_INFO_HARDWAREGAIN) | \
>>>
>>> Provide _SCALE instead of HARDWAREGAIN
>>> As it's an intensity channel (and units are tricky for color sensors given
>>> frequency dependence etc) all you need to do is ensure that if you halve
>>> the _scale and measure the same light source, the computed
>>> _RAW * _SCALE value remains constant.
>>
>> ...Which is likely to cause also the integration time setting to
>> impact the SCALE.
>>
>> You may or may not want to see the GTS-helpers
>> (drivers/iio/industrialio-gts-helper.c) - which have their own tricky
>> corners. I think Jonathan once suggested to me to keep the
>> HARDWAREGAIN as a read-only attribute to ease seeing what is going on.
>> For the last couple of days I've been reworking the BU27034 driver to
>> work with the new sensor variant - and I can definitely see the value
>> of the read-only HARDWAREGAIN when we have per channel gain settings +
>> integration time setting which all contribute to the scale...
> 
> I'm wondering if that was good advice, but it's definitely better
> than letting userspace control the gain and integration time separately

I woke up last night at 03.14 AM thinking of this :rolleyes:

> as there is no sensible way to know how to control that beyond -

I agree and disagree :)
I agree that it is simpler to just change the scale when readings get 
saturated - or when more accuracy is needed. Hence, implementing the 
scale change as is done now makes very much sense.

However, I can imagine that sometimes the measurement time plays a role 
- and people would like to have more fine grained control over things. 
In that case, if driver only allows changing things via the scale, then 
the driver is probably doing autonomous choices regarding the 
integration time - which may not be optimal for all cases (*citation 
needed).

As you may remember, I implemented the ROHM RGB and ALS sensors (the 
BU270xx series) so that the integration time can be set as well as the 
gain. These sensors (at least the BU27034, don't remember all the dirty 
details of the RGB sensors) had per-channel gain and a global 
integration time settings. Hence, the scale can be set separately for 
each channel. I invented a restriction that setting the per-channel 
scale tried to maintain the integration time and change the gain - but 
if it was not possible, the scale change changes also the integration 
time in order to yield the desired scale.

Problem was that the integration time was a global setting, and changing 
it for one channel results scale change also on the other channel(s).

To mitigate such side-effects I implemented logic that the scale change 
for other channels (caused by the integration time change) is 
compensated by changing the gain for these unrelated channels. Eg, if 
scale change for channel #1 required doubling the integration time - 
which effectively doubled the "gain contributed by integration time" 
also for the channel #2 and #3 - then the HARDWAREGAIN for the unrelated 
channels #2 and #3 is halved in order to keep their scale unchanged. Great.

Except that this is not always possible. The HWGAIN for these unrelated 
channels may have been already set to the other extreme, and further 
reducing/increasing is not possible. Or, there may be unsupported 
multipliers (gaps) in the gain range, so that setting the hardwaregain 
to required value is not possible.

Here I just decided to return an error to caller and disallow such scale 
change.

This is very much annoying solution but I ran out of good ideas. Adding 
more logic to the driver to work around this felt like asking for a 
nose-bleed. I was sure I ended up adding a bug or two, and resulting 
code that was so hairy I could never look at it again :) We can call 
that as an unmaintainable mess.

Still, what makes this even more annoying is that it might be possible 
to support the requested scale by selecting yet another integration 
time. Eg, imagine a situation where we have 2 channels. Both channels 
support gains

1x, 2x, 8x, 16x, 32x. 4x is not supported.

Let's further say we have integration times 50mS 100mS, 200mS, 400mS - 
causing "effective gains" 1x, 2x, 4x and, 8x

Now, say channel #1 is using gain 2x, channel #2 is using 8x. 
Integration time is set to 400mS.

Assume the user would like to double the scale for channel #2. This 
means the "total gain" should be halved. The HWGAIN can't be halved 
because 8x => 4x is not supported, so driver decides to drop the 
integration time from 400mS to 200mS instead. That'd do the trick.

Then the driver goes to check if the channel #1 can maintain the scale 
with this integration time. Gain caused by integration time is now 
halved so HWGAIN for channel #1 should be doubled to mitigate the 
effect. Well, the new gain for channel #1 should now go from 2x => 4x - 
which is not supported, and the driver returns error and rejects the change.

Still, the hardware could be set-up to use integration time 50mS 
(dropping the gain for channels from 8x => 1x eg. 8 times smaller), and 
channel #2 HWGAIN go from 8x => 2x (4 times smaller) thus doubling the 
scale. The channel #1 wants to maintain scale, so HWGAIN for channel #1 
should go 8 times greater, from 2x => 16x which is possible.

To make this even more annoying - the available_scales lists the 'halved 
scale' for the channel #1 as supported because there is a way to achieve 
it. So, the user can't really easily figure out what went wrong. Having 
the read-only HARDWAREGAIN and knowing the gains sensor's channels 
support would give a hint - but this is far from obvious. listing the 
supported hardwaregains might make things a bit better - but using the 
standard "available" entry in sysfs might make user to assume setting 
the hardwaregain is possible.

We may invent a new entry to list the supported hardwaregains - and I 
believe adding the logic to find supported gain-timing combinations is 
then easier (and less error-prone) in user-land applications than it is 
in driver - but I am wondering if it actually would be better just allow 
setting both the hardwaregain and integration time individually for 
those applications which may care... Well, I am probably just missing 
some culprit supporting setting the hardwaregain causes.

I believe there are many use-cases where it would work if we just 
allowed the channel #1 scale to change as a side-effect of changing 
channel #1 scale. Still, I am very reluctant to do this as there could 
be different type of data coming from these channels, and different 
consumers for this data. Allowing another application to unintentionally 
change the data for other app would in my opinion be very nasty.

The problem is not exactly urgent though and I am not working on an 
application suffering from it. But it managed to interrupt my glymphatic 
system while it was cleaning-up my brains last night. I will use it as 
an excuse if you find any errors from this babbling :)

Yours
	-- Matti

-- 
Matti Vaittinen
Linux kernel developer at ROHM Semiconductors
Oulu Finland

~~ When things go utterly wrong vim users can always type :help! ~~

Re: [PATCH v4 2/2] iio: light: ROHM BH1745 colour sensor

[Jonathan Cameron]

On Wed, 12 Jun 2024 09:07:01 +0300
Matti Vaittinen <mazziesaccount@gmail.com> wrote:

> On 6/11/24 20:14, Jonathan Cameron wrote:
> > On Mon, 10 Jun 2024 08:58:44 +0300
> > Matti Vaittinen <mazziesaccount@gmail.com> wrote:
> >   
> >> la 8. kesäk. 2024 klo 19.22 Jonathan Cameron (jic23@kernel.org) kirjoitti:  
> >>>
> >>> On Thu,  6 Jun 2024 17:29:42 +0100
> >>> Mudit Sharma <muditsharma.info@gmail.com> wrote:
> >>>     
> >>>> Add support for BH1745, which is an I2C colour sensor with red, green,
> >>>> blue and clear channels. It has a programmable active low interrupt
> >>>> pin. Interrupt occurs when the signal from the selected interrupt
> >>>> source channel crosses set interrupt threshold high or low level.
> >>>>
> >>>> This driver includes device attributes to configure the following:
> >>>> - Interrupt pin latch: The interrupt pin can be configured to
> >>>>    be latched (until interrupt register (0x60) is read or initialized)
> >>>>    or update after each measurement.
> >>>> - Interrupt source: The colour channel that will cause the interrupt
> >>>>    when channel will cross the set threshold high or low level.
> >>>>
> >>>> This driver also includes device attributes to present valid
> >>>> configuration options/values for:
> >>>> - Integration time
> >>>> - Interrupt colour source
> >>>> - Hardware gain
> >>>>     
> >>  
> >>>> +
> >>>> +#define BH1745_CHANNEL(_colour, _si, _addr)                                   \
> >>>> +     {                                                                     \
> >>>> +             .type = IIO_INTENSITY, .modified = 1,                         \
> >>>> +             .info_mask_separate = BIT(IIO_CHAN_INFO_RAW),                 \
> >>>> +             .info_mask_shared_by_type = BIT(IIO_CHAN_INFO_HARDWAREGAIN) | \  
> >>>
> >>> Provide _SCALE instead of HARDWAREGAIN
> >>> As it's an intensity channel (and units are tricky for color sensors given
> >>> frequency dependence etc) all you need to do is ensure that if you halve
> >>> the _scale and measure the same light source, the computed
> >>> _RAW * _SCALE value remains constant.  
> >>
> >> ...Which is likely to cause also the integration time setting to
> >> impact the SCALE.
> >>
> >> You may or may not want to see the GTS-helpers
> >> (drivers/iio/industrialio-gts-helper.c) - which have their own tricky
> >> corners. I think Jonathan once suggested to me to keep the
> >> HARDWAREGAIN as a read-only attribute to ease seeing what is going on.
> >> For the last couple of days I've been reworking the BU27034 driver to
> >> work with the new sensor variant - and I can definitely see the value
> >> of the read-only HARDWAREGAIN when we have per channel gain settings +
> >> integration time setting which all contribute to the scale...  
> > 
> > I'm wondering if that was good advice, but it's definitely better
> > than letting userspace control the gain and integration time separately  
> 
> I woke up last night at 03.14 AM thinking of this :rolleyes:

Ah.  I'm one for failing to get to sleep due to late night musing.
No idea if I muse because I'm not sleeping, or not sleep because
of musing!

> 
> > as there is no sensible way to know how to control that beyond -  
> 
> I agree and disagree :)
> I agree that it is simpler to just change the scale when readings get 
> saturated - or when more accuracy is needed. Hence, implementing the 
> scale change as is done now makes very much sense.
> 
> However, I can imagine that sometimes the measurement time plays a role 
> - and people would like to have more fine grained control over things. 
> In that case, if driver only allows changing things via the scale, then 
> the driver is probably doing autonomous choices regarding the 
> integration time - which may not be optimal for all cases (*citation 
> needed).

Agreed even without the complexity you mention later- there will be cases
where people want ugly (noisy) data quickly so will crank the gain up
to reduce the integration time.
How often they apply to light sensors is an interesting question.

> 
> As you may remember, I implemented the ROHM RGB and ALS sensors (the 
> BU270xx series) so that the integration time can be set as well as the 
> gain. These sensors (at least the BU27034, don't remember all the dirty 
> details of the RGB sensors) had per-channel gain and a global 
> integration time settings. Hence, the scale can be set separately for 
> each channel. I invented a restriction that setting the per-channel 
> scale tried to maintain the integration time and change the gain - but 
> if it was not possible, the scale change changes also the integration 
> time in order to yield the desired scale.
> 
> Problem was that the integration time was a global setting, and changing 
> it for one channel results scale change also on the other channel(s).
> 
> To mitigate such side-effects I implemented logic that the scale change 
> for other channels (caused by the integration time change) is 
> compensated by changing the gain for these unrelated channels. Eg, if 
> scale change for channel #1 required doubling the integration time - 
> which effectively doubled the "gain contributed by integration time" 
> also for the channel #2 and #3 - then the HARDWAREGAIN for the unrelated 
> channels #2 and #3 is halved in order to keep their scale unchanged. Great.
> 
> Except that this is not always possible. The HWGAIN for these unrelated 
> channels may have been already set to the other extreme, and further 
> reducing/increasing is not possible. Or, there may be unsupported 
> multipliers (gaps) in the gain range, so that setting the hardwaregain 
> to required value is not possible.
> 
> Here I just decided to return an error to caller and disallow such scale 
> change.
> 
> This is very much annoying solution but I ran out of good ideas. Adding 
> more logic to the driver to work around this felt like asking for a 
> nose-bleed. I was sure I ended up adding a bug or two, and resulting 
> code that was so hairy I could never look at it again :) We can call 
> that as an unmaintainable mess.
> 

Yeah. I vaguely recall this one was a bit nasty and result wasn't
entirely satisfying.

> Still, what makes this even more annoying is that it might be possible 
> to support the requested scale by selecting yet another integration 
> time. Eg, imagine a situation where we have 2 channels. Both channels 
> support gains
> 
> 1x, 2x, 8x, 16x, 32x. 4x is not supported.
> 
> Let's further say we have integration times 50mS 100mS, 200mS, 400mS - 
> causing "effective gains" 1x, 2x, 4x and, 8x
> 
> Now, say channel #1 is using gain 2x, channel #2 is using 8x. 
> Integration time is set to 400mS.
> 
> Assume the user would like to double the scale for channel #2. This 
> means the "total gain" should be halved. The HWGAIN can't be halved 
> because 8x => 4x is not supported, so driver decides to drop the 
> integration time from 400mS to 200mS instead. That'd do the trick.
> 
> Then the driver goes to check if the channel #1 can maintain the scale 
> with this integration time. Gain caused by integration time is now 
> halved so HWGAIN for channel #1 should be doubled to mitigate the 
> effect. Well, the new gain for channel #1 should now go from 2x => 4x - 
> which is not supported, and the driver returns error and rejects the change.
> 
> Still, the hardware could be set-up to use integration time 50mS 
> (dropping the gain for channels from 8x => 1x eg. 8 times smaller), and 
> channel #2 HWGAIN go from 8x => 2x (4 times smaller) thus doubling the 
> scale. The channel #1 wants to maintain scale, so HWGAIN for channel #1 
> should go 8 times greater, from 2x => 16x which is possible.
> 
> To make this even more annoying - the available_scales lists the 'halved 
> scale' for the channel #1 as supported because there is a way to achieve 
> it. So, the user can't really easily figure out what went wrong. Having 
> the read-only HARDWAREGAIN and knowing the gains sensor's channels 
> support would give a hint - but this is far from obvious. listing the 
> supported hardwaregains might make things a bit better - but using the 
> standard "available" entry in sysfs might make user to assume setting 
> the hardwaregain is possible.

That would be an odd bit of interface indeed.

> 
> We may invent a new entry to list the supported hardwaregains - and I 
> believe adding the logic to find supported gain-timing combinations is 
> then easier (and less error-prone) in user-land applications than it is 
> in driver - but I am wondering if it actually would be better just allow 
> setting both the hardwaregain and integration time individually for 
> those applications which may care... Well, I am probably just missing 
> some culprit supporting setting the hardwaregain causes.

We could do something similar to what we did recently for a power mode
switch on an IMU.  The interface is also less than ideal though but
was exploring a similar problem:
[PATCH v4 2/2] iio: imu: inv_icm42600: add support of accel low-power mode
https://lore.kernel.org/all/20240605195949.766677-3-inv.git-commit@tdk.com/

That was much simpler than this. The device has two power modes
(trading off power vs noise).  The lowest sampling frequencies only
worked in low power mode and the highest only in low noise mode.
A few in the middle were available in both modes.  We defaulted to
choosing low power if available.  The aim was to design an interface
where everything worked as normal if you didn't grab the 'expert'
controls. So it defaults to a preference for low power (here
equivalent would be defaults to lowest hardware gain) but we provided
an ability to override - if possible. So you could specify what you
wanted the gain to be and if that was possible whilst retaining the
sampling frequency (here that would be the scale) then the change
would be made. If not it wouldn't an reading the mode back (here
that would be reading hardware gain) would return the actual setting
achieved.  In that case the power mode setting isn't sticky. To enter
the low noise mode you have to be at a sampling frequency where it
is supported.  That sort of restriction might not work here.

In that case the control grabbed to override the power mode is not
standard ABI so we an be fairly sure no normal software will tweak
it but in the rare occasion where a user needs it the tweak is
available.

Here we might need a similar 'out of ABI' trick to make it clear that
scale is the main control to use.

> 
> I believe there are many use-cases where it would work if we just 
> allowed the channel #1 scale to change as a side-effect of changing 
> channel #1 scale. Still, I am very reluctant to do this as there could 
> be different type of data coming from these channels, and different 
> consumers for this data. Allowing another application to unintentionally 
> change the data for other app would in my opinion be very nasty.

You've lost me here.

> 
> The problem is not exactly urgent though and I am not working on an 
> application suffering from it. But it managed to interrupt my glymphatic 
> system while it was cleaning-up my brains last night. I will use it as 
> an excuse if you find any errors from this babbling :)

For this complexity I definitely want a known user who cares.
It's complex and we'd need to construct the userspace to use it.

For the IMU analogy above, it is easier to understand that use case.

Gut feeling is normally people are actually cranking scaling of light
channels up and down together as hopefully they are approximately balanced
for 'white' giving similar scales on all sensors (by filters or fixed gains)
and people would only need to care if they were trying to measure a weak
blue signal in a red world. If we have a case that doesn't work well
for that sort of global scaling (I can sort of see that as a possible
problem due to the transition states not being possible) then we
should make sure that one works!

Jonathan
 
> 
> Yours
> 	-- Matti
> 

Re: [PATCH v4 2/2] iio: light: ROHM BH1745 colour sensor

[Matti Vaittinen]

On 6/15/24 21:23, Jonathan Cameron wrote:
> On Wed, 12 Jun 2024 09:07:01 +0300
> Matti Vaittinen <mazziesaccount@gmail.com> wrote:
> 
>> On 6/11/24 20:14, Jonathan Cameron wrote:
>>> On Mon, 10 Jun 2024 08:58:44 +0300
>>> Matti Vaittinen <mazziesaccount@gmail.com> wrote:
>>>    
>>>> la 8. kesäk. 2024 klo 19.22 Jonathan Cameron (jic23@kernel.org) kirjoitti:
>>>>>
>>>>> On Thu,  6 Jun 2024 17:29:42 +0100
>>>>> Mudit Sharma <muditsharma.info@gmail.com> wrote:
>>>>>      
>>>>>> Add support for BH1745, which is an I2C colour sensor with red, green,
>>>>>> blue and clear channels. It has a programmable active low interrupt
>>>>>> pin. Interrupt occurs when the signal from the selected interrupt
>>>>>> source channel crosses set interrupt threshold high or low level.
>>>>>>
>>>>>> This driver includes device attributes to configure the following:
>>>>>> - Interrupt pin latch: The interrupt pin can be configured to
>>>>>>     be latched (until interrupt register (0x60) is read or initialized)
>>>>>>     or update after each measurement.
>>>>>> - Interrupt source: The colour channel that will cause the interrupt
>>>>>>     when channel will cross the set threshold high or low level.
>>>>>>
>>>>>> This driver also includes device attributes to present valid
>>>>>> configuration options/values for:
>>>>>> - Integration time
>>>>>> - Interrupt colour source
>>>>>> - Hardware gain
>>>>>>      
>>>>   
>>>>>> +
>>>>>> +#define BH1745_CHANNEL(_colour, _si, _addr)                                   \
>>>>>> +     {                                                                     \
>>>>>> +             .type = IIO_INTENSITY, .modified = 1,                         \
>>>>>> +             .info_mask_separate = BIT(IIO_CHAN_INFO_RAW),                 \
>>>>>> +             .info_mask_shared_by_type = BIT(IIO_CHAN_INFO_HARDWAREGAIN) | \
>>>>>
>>>>> Provide _SCALE instead of HARDWAREGAIN
>>>>> As it's an intensity channel (and units are tricky for color sensors given
>>>>> frequency dependence etc) all you need to do is ensure that if you halve
>>>>> the _scale and measure the same light source, the computed
>>>>> _RAW * _SCALE value remains constant.
>>>>
>>>> ...Which is likely to cause also the integration time setting to
>>>> impact the SCALE.
>>>>
>>>> You may or may not want to see the GTS-helpers
>>>> (drivers/iio/industrialio-gts-helper.c) - which have their own tricky
>>>> corners. I think Jonathan once suggested to me to keep the
>>>> HARDWAREGAIN as a read-only attribute to ease seeing what is going on.
>>>> For the last couple of days I've been reworking the BU27034 driver to
>>>> work with the new sensor variant - and I can definitely see the value
>>>> of the read-only HARDWAREGAIN when we have per channel gain settings +
>>>> integration time setting which all contribute to the scale...
>>>
>>> I'm wondering if that was good advice, but it's definitely better
>>> than letting userspace control the gain and integration time separately
>>
>> I woke up last night at 03.14 AM thinking of this :rolleyes:
> 
> Ah.  I'm one for failing to get to sleep due to late night musing.
> No idea if I muse because I'm not sleeping, or not sleep because
> of musing!

Either way, not getting sleep at night is rarely amusing. :/ Sometimes 
it is actually one of the early signs of a burn out.

>>> as there is no sensible way to know how to control that beyond -
>>
>> I agree and disagree :)
>> I agree that it is simpler to just change the scale when readings get
>> saturated - or when more accuracy is needed. Hence, implementing the
>> scale change as is done now makes very much sense.
>>
>> However, I can imagine that sometimes the measurement time plays a role
>> - and people would like to have more fine grained control over things.
>> In that case, if driver only allows changing things via the scale, then
>> the driver is probably doing autonomous choices regarding the
>> integration time - which may not be optimal for all cases (*citation
>> needed).
> 
> Agreed even without the complexity you mention later- there will be cases
> where people want ugly (noisy) data quickly so will crank the gain up
> to reduce the integration time.
> How often they apply to light sensors is an interesting question.
> 
>>
>> As you may remember, I implemented the ROHM RGB and ALS sensors (the
>> BU270xx series) so that the integration time can be set as well as the
>> gain. These sensors (at least the BU27034, don't remember all the dirty
>> details of the RGB sensors) had per-channel gain and a global
>> integration time settings. Hence, the scale can be set separately for
>> each channel. I invented a restriction that setting the per-channel
>> scale tried to maintain the integration time and change the gain - but
>> if it was not possible, the scale change changes also the integration
>> time in order to yield the desired scale.
>>
>> Problem was that the integration time was a global setting, and changing
>> it for one channel results scale change also on the other channel(s).
>>
>> To mitigate such side-effects I implemented logic that the scale change
>> for other channels (caused by the integration time change) is
>> compensated by changing the gain for these unrelated channels. Eg, if
>> scale change for channel #1 required doubling the integration time -
>> which effectively doubled the "gain contributed by integration time"
>> also for the channel #2 and #3 - then the HARDWAREGAIN for the unrelated
>> channels #2 and #3 is halved in order to keep their scale unchanged. Great.
>>
>> Except that this is not always possible. The HWGAIN for these unrelated
>> channels may have been already set to the other extreme, and further
>> reducing/increasing is not possible. Or, there may be unsupported
>> multipliers (gaps) in the gain range, so that setting the hardwaregain
>> to required value is not possible.
>>
>> Here I just decided to return an error to caller and disallow such scale
>> change.
>>
>> This is very much annoying solution but I ran out of good ideas. Adding
>> more logic to the driver to work around this felt like asking for a
>> nose-bleed. I was sure I ended up adding a bug or two, and resulting
>> code that was so hairy I could never look at it again :) We can call
>> that as an unmaintainable mess.
>>
> 
> Yeah. I vaguely recall this one was a bit nasty and result wasn't
> entirely satisfying.
> 
>> Still, what makes this even more annoying is that it might be possible
>> to support the requested scale by selecting yet another integration
>> time. Eg, imagine a situation where we have 2 channels. Both channels
>> support gains
>>
>> 1x, 2x, 8x, 16x, 32x. 4x is not supported.
>>
>> Let's further say we have integration times 50mS 100mS, 200mS, 400mS -
>> causing "effective gains" 1x, 2x, 4x and, 8x
>>
>> Now, say channel #1 is using gain 2x, channel #2 is using 8x.
>> Integration time is set to 400mS.
>>
>> Assume the user would like to double the scale for channel #2. This
>> means the "total gain" should be halved. The HWGAIN can't be halved
>> because 8x => 4x is not supported, so driver decides to drop the
>> integration time from 400mS to 200mS instead. That'd do the trick.
>>
>> Then the driver goes to check if the channel #1 can maintain the scale
>> with this integration time. Gain caused by integration time is now
>> halved so HWGAIN for channel #1 should be doubled to mitigate the
>> effect. Well, the new gain for channel #1 should now go from 2x => 4x -
>> which is not supported, and the driver returns error and rejects the change.
>>
>> Still, the hardware could be set-up to use integration time 50mS
>> (dropping the gain for channels from 8x => 1x eg. 8 times smaller), and
>> channel #2 HWGAIN go from 8x => 2x (4 times smaller) thus doubling the
>> scale. The channel #1 wants to maintain scale, so HWGAIN for channel #1
>> should go 8 times greater, from 2x => 16x which is possible.
>>
>> To make this even more annoying - the available_scales lists the 'halved
>> scale' for the channel #1 as supported because there is a way to achieve
>> it. So, the user can't really easily figure out what went wrong. Having
>> the read-only HARDWAREGAIN and knowing the gains sensor's channels
>> support would give a hint - but this is far from obvious. listing the
>> supported hardwaregains might make things a bit better - but using the
>> standard "available" entry in sysfs might make user to assume setting
>> the hardwaregain is possible.
> 
> That would be an odd bit of interface indeed.
> 
>>
>> We may invent a new entry to list the supported hardwaregains - and I
>> believe adding the logic to find supported gain-timing combinations is
>> then easier (and less error-prone) in user-land applications than it is
>> in driver - but I am wondering if it actually would be better just allow
>> setting both the hardwaregain and integration time individually for
>> those applications which may care... Well, I am probably just missing
>> some culprit supporting setting the hardwaregain causes.
> 
> We could do something similar to what we did recently for a power mode
> switch on an IMU.  The interface is also less than ideal though but
> was exploring a similar problem:
> [PATCH v4 2/2] iio: imu: inv_icm42600: add support of accel low-power mode
> https://lore.kernel.org/all/20240605195949.766677-3-inv.git-commit@tdk.com/

I took a quick look at this. So, changing the "power-mode" enum enables 
doing the not-so-standard changes.

> That was much simpler than this. The device has two power modes
> (trading off power vs noise).  The lowest sampling frequencies only
> worked in low power mode and the highest only in low noise mode.
> A few in the middle were available in both modes.  We defaulted to
> choosing low power if available.  The aim was to design an interface
> where everything worked as normal if you didn't grab the 'expert'
> controls. So it defaults to a preference for low power (here
> equivalent would be defaults to lowest hardware gain) but we provided
> an ability to override - if possible. So you could specify what you
> wanted the gain to be and if that was possible whilst retaining the
> sampling frequency (here that would be the scale) then the change
> would be made. If not it wouldn't an reading the mode back (here
> that would be reading hardware gain) would return the actual setting
> achieved.  In that case the power mode setting isn't sticky. To enter
> the low noise mode you have to be at a sampling frequency where it
> is supported.  That sort of restriction might not work here.
> 
> In that case the control grabbed to override the power mode is not
> standard ABI so we an be fairly sure no normal software will tweak
> it but in the rare occasion where a user needs it the tweak is
> available.
> 
> Here we might need a similar 'out of ABI' trick to make it clear that
> scale is the main control to use.

Thank you for the ideas. Adding a entry to enable 'not standard 
settings' definitely sounds like a way to explore. I think I'll return 
to this later :)

>> I believe there are many use-cases where it would work if we just
>> allowed the channel #1 scale to change as a side-effect of changing
>> channel #1 scale. Still, I am very reluctant to do this as there could
>> be different type of data coming from these channels, and different
>> consumers for this data. Allowing another application to unintentionally
>> change the data for other app would in my opinion be very nasty.
> 
> You've lost me here.

Sorry. I did not really manage to explain it too well.

Basically, I just further pondered handling the the case where:
- a requested change of channel #A scale could not be done purely by 
changing the gain but would require an integration time change.
AND
- the channel #B gain change caused by integration time change could not 
be compensated by changing #B hardwaregain.

I was just saying that allowing this channel #A scale change even though 
it would also impact the scale of channel #B would probably be Ok for 
many users. I think that a few of the users could be prepared for other 
channels to change as well, and go read back all channels' scales after 
changing one.

Then I was further speculating that there might be cases where channel 
#A data and channel #B data were consumed by different applications. It 
was all just speculation. For example, the original BU27034 had two 
channels for visible light and one for IR. So I built an imaginary 
device which ran two different user applications, one interested on 
visible light for darkening my sunglasses and the other interested on IR 
channel and then toggling the cooling fan to keep my vacation drinks at 
optimum temperature. :)

Here, if the sunglasses application changing the scale for visible light 
caused also the scale of the IR channel to change, my fan-application 
would unexpectedly start pick up differently scaled values and the 
temperature of my drinks would be all wrong.

>> The problem is not exactly urgent though and I am not working on an
>> application suffering from it. But it managed to interrupt my glymphatic
>> system while it was cleaning-up my brains last night. I will use it as
>> an excuse if you find any errors from this babbling :)
> 
> For this complexity I definitely want a known user who cares.
> It's complex and we'd need to construct the userspace to use it.

Agreed. It's nice someone drops me back on earth when I start drifting 
too far :) Thanks!

> Gut feeling is normally people are actually cranking scaling of light
> channels up and down together as hopefully they are approximately balanced
> for 'white' giving similar scales on all sensors (by filters or fixed gains)

I appreciate your insight on how people usually use these devices :) 
It's very valuable to me.

> and people would only need to care if they were trying to measure a weak
> blue signal in a red world. If we have a case that doesn't work well
> for that sort of global scaling (I can sort of see that as a possible
> problem due to the transition states not being possible) then we
> should make sure that one works!

Yes. I think some users will eventually hit to a scale transition which 
will be NACKed by the driver. Also, I don't think this problem will be 
specific to the BU27034 sensor, but in some form this will be possible 
for many gain-time-scale type devices. I just don't have a good generic 
solution in my mind right now.

Oh, besides, it seems raining stopped. Time to turn off my computer and 
go out to the yard :)

Yours,
	-- Matti

-- 
Matti Vaittinen
Linux kernel developer at ROHM Semiconductors
Oulu Finland

~~ When things go utterly wrong vim users can always type :help! ~~

Re: [PATCH v4 2/2] iio: light: ROHM BH1745 colour sensor

[Jonathan Cameron]

> > Gut feeling is normally people are actually cranking scaling of light
> > channels up and down together as hopefully they are approximately balanced
> > for 'white' giving similar scales on all sensors (by filters or fixed gains)  
> 
> I appreciate your insight on how people usually use these devices :) 
> It's very valuable to me.

Pah, never trust me on this stuff.  I've not been on the 'ground' with
real sensor for a long time and even then my use cases were far from typical.

These days I just play with big toys (though having just helped moved them
between buildings this week I can say that they are at least real and not all
in the cloud).

> 
> > and people would only need to care if they were trying to measure a weak
> > blue signal in a red world. If we have a case that doesn't work well
> > for that sort of global scaling (I can sort of see that as a possible
> > problem due to the transition states not being possible) then we
> > should make sure that one works!  
> 
> Yes. I think some users will eventually hit to a scale transition which 
> will be NACKed by the driver. Also, I don't think this problem will be 
> specific to the BU27034 sensor, but in some form this will be possible 
> for many gain-time-scale type devices. I just don't have a good generic 
> solution in my mind right now.
> 
> Oh, besides, it seems raining stopped. Time to turn off my computer and 
> go out to the yard :)

Wise!


> 
> Yours,
> 	-- Matti
> 

Re: [PATCH v4 2/2] iio: light: ROHM BH1745 colour sensor

[Mudit Sharma]

On 10/06/2024 06:58, Matti Vaittinen wrote:
> la 8. kesäk. 2024 klo 19.22 Jonathan Cameron (jic23@kernel.org) kirjoitti:
>>
>> On Thu,  6 Jun 2024 17:29:42 +0100
>> Mudit Sharma <muditsharma.info@gmail.com> wrote:
>>
>>> Add support for BH1745, which is an I2C colour sensor with red, green,
>>> blue and clear channels. It has a programmable active low interrupt
>>> pin. Interrupt occurs when the signal from the selected interrupt
>>> source channel crosses set interrupt threshold high or low level.
>>>
>>> This driver includes device attributes to configure the following:
>>> - Interrupt pin latch: The interrupt pin can be configured to
>>>    be latched (until interrupt register (0x60) is read or initialized)
>>>    or update after each measurement.
>>> - Interrupt source: The colour channel that will cause the interrupt
>>>    when channel will cross the set threshold high or low level.
>>>
>>> This driver also includes device attributes to present valid
>>> configuration options/values for:
>>> - Integration time
>>> - Interrupt colour source
>>> - Hardware gain
>>>
> 
>>> +
>>> +#define BH1745_CHANNEL(_colour, _si, _addr)                                   \
>>> +     {                                                                     \
>>> +             .type = IIO_INTENSITY, .modified = 1,                         \
>>> +             .info_mask_separate = BIT(IIO_CHAN_INFO_RAW),                 \
>>> +             .info_mask_shared_by_type = BIT(IIO_CHAN_INFO_HARDWAREGAIN) | \
>>
>> Provide _SCALE instead of HARDWAREGAIN
>> As it's an intensity channel (and units are tricky for color sensors given
>> frequency dependence etc) all you need to do is ensure that if you halve
>> the _scale and measure the same light source, the computed
>> _RAW * _SCALE value remains constant.
> 
> ...Which is likely to cause also the integration time setting to
> impact the SCALE.
> 
> You may or may not want to see the GTS-helpers
> (drivers/iio/industrialio-gts-helper.c) - which have their own tricky
> corners. I think Jonathan once suggested to me to keep the

Hi Matti,

Thank you for the recommendation here.

Looking into GTS-helper for v5.

Best regards,
Mudit Sharma

Re: [PATCH v4 2/2] iio: light: ROHM BH1745 colour sensor

[Mudit Sharma]

On 08/06/2024 17:22, Jonathan Cameron wrote:
> On Thu,  6 Jun 2024 17:29:42 +0100
> 
> Hi Mudit,
> 
> Welcome to IIO.
> 
> Some comments inline.  Some apply more widely than where I've
> called them out, so please look for similar cases and tidy them all
> up for v5.
> 
> Thanks,
> 
> Jonathan
> 

Hi Jonathan,

Thank you for the review and suggestions on this.

I'll address the comments and include related fixes in v5.

Best regards,
Mudit Sharma

Re: [PATCH v4 1/2] dt-bindings: iio: light: ROHM BH1745

[Jonathan Cameron]

On Thu,  6 Jun 2024 17:29:41 +0100
Mudit Sharma <muditsharma.info@gmail.com> wrote:

> Add ROHM BH1745 - 4 channel I2C colour sensor's dt-bindings.
> 
> Signed-off-by: Mudit Sharma <muditsharma.info@gmail.com>
> Reviewed-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
Hi Mudit,

I'd like to see the vcc-supply in here from the start.

Thanks,

Jonathan


> ---
> v3->v4:
> - No changes
> v2->v3:
> - Move 'additionalProperties' after 'required' block
> - Remove block style indicator '|' from description
> v1->v2:
> - Fix yaml issue: Make `maintainers` a list
> 
>  .../bindings/iio/light/rohm,bh1745.yaml       | 49 +++++++++++++++++++
>  1 file changed, 49 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/iio/light/rohm,bh1745.yaml
> 
> diff --git a/Documentation/devicetree/bindings/iio/light/rohm,bh1745.yaml b/Documentation/devicetree/bindings/iio/light/rohm,bh1745.yaml
> new file mode 100644
> index 000000000000..d5338a0af6b3
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/iio/light/rohm,bh1745.yaml
> @@ -0,0 +1,49 @@
> +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/iio/light/rohm,bh1745.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: ROHM BH1745 colour sensor
> +
> +maintainers:
> +  - Mudit Sharma <muditsharma.info@gmail.com>
> +
> +description:
> +  BH1745 is an I2C colour sensor with red, green, blue and clear
> +  channels. It has a programmable active low interrupt pin.
> +  Interrupt occurs when the signal from the selected interrupt
> +  source channel crosses set interrupt threshold high/low level.
> +
> +properties:
> +  compatible:
> +    const: rohm,bh1745
> +
> +  reg:
> +    maxItems: 1
> +
> +  interrupts:
> +    maxItems: 1
There are some historical bindings that are missing supplies, but
for everything recent we have tried to document them from the first.
So this should have

  vcc-supply: true

> +
> +required:
> +  - compatible
> +  - reg
    - vcc-supply

Even though the regulator framework will provide you with a dummy regulator,
the binding reflects that the device definitely needs power.

Note that there are nice devm_regulator_get_enable() and similar to deal
with turning the power on at driver load (I guess that doesn't do anything on
your board) and off at driver remove.  Ideally the driver should use
that.  There have been many occasions in the past (all a while back)
where we didn't add power supply control from the start and had to add
it soon after when it turned out some board didn't default to power
on for the sensors.

Thanks,

Jonathan

> +
> +additionalProperties: false
> +
> +examples:
> +  - |
> +    #include <dt-bindings/interrupt-controller/irq.h>
> +    i2c {
> +        #address-cells = <1>;
> +        #size-cells = <0>;
> +
> +        colour-sensor@38 {
> +            compatible = "rohm,bh1745";
> +            reg = <0x38>;
> +            interrupt-parent = <&gpio>;
> +            interrupts = <19 IRQ_TYPE_LEVEL_LOW>;
> +        };
> +    };
> +
> +...