[PATCH v2 1/3] dt-bindings: iio: light: ROHM BH1745

[PATCH v2 1/3] 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>
---
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..ac5c4d160513
--- /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
+
+additionalProperties: false
+
+required:
+  - compatible
+  - reg
+
+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 v2 2/3] 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

Signed-off-by: Mudit Sharma <muditsharma.info@gmail.com>
---
v1->v2:
- No changes

 drivers/iio/light/Kconfig  |  12 +
 drivers/iio/light/Makefile |   1 +
 drivers/iio/light/bh1745.c | 879 +++++++++++++++++++++++++++++++++++++
 3 files changed, 892 insertions(+)
 create mode 100644 drivers/iio/light/bh1745.c

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..a7b660a1bdc8
--- /dev/null
+++ b/drivers/iio/light/bh1745.c
@@ -0,0 +1,879 @@
+// 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_RED,
+	BH1745_INT_SOURCE_GREEN,
+	BH1745_INT_SOURCE_BLUE,
+	BH1745_INT_SOURCE_CLEAR,
+} bh1745_int_source;
+
+enum {
+	BH1745_ADC_GAIN_1X,
+	BH1745_ADC_GAIN_2X,
+	BH1745_ADC_GAIN_16X,
+} bh1745_gain;
+
+enum {
+	BH1745_MEASUREMENT_TIME_160MS,
+	BH1745_MEASUREMENT_TIME_320MS,
+	BH1745_MEASUREMENT_TIME_640MS,
+	BH1745_MEASUREMENT_TIME_1280MS,
+	BH1745_MEASUREMENT_TIME_2560MS,
+	BH1745_MEASUREMENT_TIME_5120MS,
+} bh1745_measurement_time;
+
+enum {
+	BH1745_PRESISTENCE_UPDATE_TOGGLE,
+	BH1745_PRESISTENCE_UPDATE_EACH_MEASUREMENT,
+	BH1745_PRESISTENCE_UPDATE_FOUR_MEASUREMENT,
+	BH1745_PRESISTENCE_UPDATE_EIGHT_MEASUREMENT,
+} bh1745_presistence_value;
+
+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 = 0;
+
+	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;
+	}
+	return ret;
+}
+
+static int bh1745_read_value(struct bh1745_data *data, u8 reg, void *value,
+			     size_t len)
+{
+	int ret = 0;
+
+	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;
+	}
+	return ret;
+}
+
+static ssize_t in_interrupt_source_show(struct device *dev,
+					struct device_attribute *attr,
+					char *buf)
+{
+	int ret = 0;
+	int value = 0;
+	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 = 0;
+	u16 value = 0;
+	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;
+	}
+	mutex_lock(&data->lock);
+	data->int_src = value;
+	value = value << 2;
+	ret = bh1745_read_value(data, BH1745_INTR, &data->interrupt, 1);
+	if (ret < 0) {
+		mutex_unlock(&data->lock);
+		return ret;
+	}
+	data->interrupt &= ~BH1745_INT_SOURCE_MASK;
+	data->interrupt |= value;
+	ret = bh1745_write_value(data, BH1745_INTR, &data->interrupt, 1);
+	mutex_unlock(&data->lock);
+	if (ret < 0)
+		return ret;
+	return len;
+}
+
+static ssize_t in_interrupt_latch_show(struct device *dev,
+				       struct device_attribute *attr, char *buf)
+{
+	int value = 0;
+	struct iio_dev *indio_dev = dev_to_iio_dev(dev);
+	struct bh1745_data *data = iio_priv(indio_dev);
+
+	bh1745_read_value(data, BH1745_INTR, &value, 1);
+
+	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 = 0;
+	u16 value = 0;
+	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 == 0) {
+		data->int_latch = value;
+		mutex_lock(&data->lock);
+		ret = bh1745_read_value(data, BH1745_INTR, &data->interrupt, 1);
+		if (ret < 0) {
+			mutex_unlock(&data->lock);
+			return ret;
+		}
+
+		data->interrupt &= ~BH1745_INT_SIGNAL_LATCHED;
+		ret = bh1745_write_value(data, BH1745_INTR, &data->interrupt,
+					 1);
+		mutex_unlock(&data->lock);
+		if (ret < 0)
+			return ret;
+
+	} else if (value == 1) {
+		data->int_latch = value;
+		mutex_lock(&data->lock);
+		ret = bh1745_read_value(data, BH1745_INTR, &data->interrupt, 1);
+		if (ret < 0) {
+			mutex_unlock(&data->lock);
+			return ret;
+		}
+
+		data->interrupt |= BH1745_INT_SIGNAL_LATCHED;
+		ret = bh1745_write_value(data, BH1745_INTR, &data->interrupt,
+					 1);
+		mutex_unlock(&data->lock);
+		if (ret < 0)
+			return ret;
+
+	} else {
+		dev_err(dev,
+			"Value out of range for latch setup. Supported values '0' or '1'\n");
+	}
+	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 = 0;
+	u8 value = 0;
+
+	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 = 0;
+	u8 value = 0;
+
+	ret = bh1745_read_value(data, BH1745_MODE_CTRL_2, &value, 1);
+	if (ret < 0)
+		return ret;
+
+	value |= BH1745_RGBC_EN;
+	mutex_lock(&data->lock);
+	data->mode_ctrl2 = value;
+	ret = bh1745_write_value(data, BH1745_MODE_CTRL_2, &data->mode_ctrl2,
+				 1);
+	mutex_unlock(&data->lock);
+
+	return ret;
+}
+
+static int bh1745_power_off(struct bh1745_data *data)
+{
+	int ret = 0;
+	int value = 0;
+
+	ret = bh1745_read_value(data, BH1745_MODE_CTRL_2, &value, 1);
+	if (ret < 0)
+		return ret;
+
+	value &= ~BH1745_RGBC_EN;
+	mutex_lock(&data->lock);
+	data->mode_ctrl2 = value;
+	ret = bh1745_write_value(data, BH1745_MODE_CTRL_2, &data->mode_ctrl2,
+				 1);
+	mutex_unlock(&data->lock);
+	return ret;
+}
+
+static int bh1745_set_int_time(struct bh1745_data *data, int val, int val2)
+{
+	int ret = 0;
+
+	for (u8 i = 0; i < ARRAY_SIZE(bh1745_int_time); i++) {
+		if (val == bh1745_int_time[i][0] &&
+		    val2 == bh1745_int_time[i][1]) {
+			mutex_lock(&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);
+			mutex_unlock(&data->lock);
+			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 = 0;
+	u16 value = 0;
+
+	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: {
+		mutex_lock(&data->lock);
+		ret = bh1745_read_value(data, BH1745_MODE_CTRL_2,
+					&data->mode_ctrl2, 1);
+		if (ret < 0) {
+			mutex_unlock(&data->lock);
+			return ret;
+		}
+
+		value = data->mode_ctrl2 & BH1745_ADC_GAIN_MASK;
+		mutex_unlock(&data->lock);
+
+		*val = bh1745_gain_factor[value];
+		return IIO_VAL_INT;
+	}
+
+	case IIO_CHAN_INFO_INT_TIME: {
+		mutex_lock(&data->lock);
+		ret = bh1745_read_value(data, BH1745_MODE_CTRL_1,
+					&data->mode_ctrl1, 1);
+		if (ret < 0) {
+			mutex_unlock(&data->lock);
+			return ret;
+		}
+		value = data->mode_ctrl1 & BH1745_MEASUREMENT_TIME_MASK;
+		mutex_unlock(&data->lock);
+
+		*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 = 0;
+
+	switch (mask) {
+	case IIO_CHAN_INFO_HARDWAREGAIN: {
+		for (u8 i = 0; i < ARRAY_SIZE(bh1745_gain_factor); i++) {
+			if (bh1745_gain_factor[i] == val) {
+				mutex_lock(&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);
+				mutex_unlock(&data->lock);
+				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 = 0;
+
+	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 = 0;
+
+	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 = 0;
+	u8 val = 0;
+	struct iio_dev *indio_dev = iio_trigger_get_drvdata(trig);
+	struct bh1745_data *data = iio_priv(indio_dev);
+
+	if (state) {
+		mutex_lock(&data->lock);
+		ret = bh1745_read_value(data, BH1745_INTR, &val, 1);
+		val |= (BH1745_INT_ENABLE |
+			(data->int_latch << BH1745_INT_SIGNAL_LATCH_OFFSET) |
+			(data->int_src << BH1745_INT_SOURCE_OFFSET));
+		data->interrupt = val;
+		ret = bh1745_write_value(data, BH1745_INTR, &data->interrupt,
+					 1);
+		mutex_unlock(&data->lock);
+	} else {
+		mutex_lock(&data->lock);
+		data->interrupt = val;
+		ret = bh1745_write_value(data, BH1745_INTR, &data->interrupt,
+					 1);
+		mutex_unlock(&data->lock);
+	}
+	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 = 0;
+	int value = 0;
+
+	ret = bh1745_read_value(data, BH1745_INTR, &value, 1);
+	if (ret < 0)
+		return ret;
+
+	if (value & BH1745_INTR_STATUS) {
+		mutex_lock(&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));
+
+		mutex_unlock(&data->lock);
+		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);
+	u16 value = 0;
+	struct {
+		u16 chans[4];
+
+		s64 timestamp __aligned(8);
+	} scan;
+
+	int i, j = 0;
+
+	for_each_set_bit(i, indio_dev->active_scan_mask,
+			 indio_dev->masklength) {
+		bh1745_read_value(data, BH1745_R_LSB + 2 * i, &value, 2);
+		scan.chans[j++] = value;
+	}
+
+	iio_push_to_buffers_with_timestamp(indio_dev, &scan,
+					   iio_get_time_ns(indio_dev));
+
+	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 = 0;
+
+	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 = 0;
+	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_MOD_NAME;
+	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");
+
+	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

[PATCH v2 3/3] MAINTAINERS: Add maintainer for ROHM BH1745

[Mudit Sharma]

Add myself as maintainer for ROHM BH1745 colour sensor driver.

Signed-off-by: Mudit Sharma <muditsharma.info@gmail.com>
---
v1->v2:
- No changes

 MAINTAINERS | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/MAINTAINERS b/MAINTAINERS
index d6c90161c7bf..945873321fef 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -19407,6 +19407,13 @@ 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:	Documentation/devicetree/bindings/iio/light/rohm,bh1745.yaml
+F:	drivers/iio/light/bh1745.c
+
 ROHM BH1750 AMBIENT LIGHT SENSOR DRIVER
 M:	Tomasz Duszynski <tduszyns@gmail.com>
 S:	Maintained
-- 
2.43.0

Re: [PATCH v2 3/3] MAINTAINERS: Add maintainer for ROHM BH1745

[Ivan Orlov]

On 6/3/24 17:21, Mudit Sharma wrote:
> Add myself as maintainer for ROHM BH1745 colour sensor driver.
> 
> Signed-off-by: Mudit Sharma <muditsharma.info@gmail.com>
> ---
> v1->v2:
> - No changes
> 
>   MAINTAINERS | 7 +++++++
>   1 file changed, 7 insertions(+)
> 
> diff --git a/MAINTAINERS b/MAINTAINERS
> index d6c90161c7bf..945873321fef 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -19407,6 +19407,13 @@ 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:	Documentation/devicetree/bindings/iio/light/rohm,bh1745.yaml
> +F:	drivers/iio/light/bh1745.c
> +
>   ROHM BH1750 AMBIENT LIGHT SENSOR DRIVER
>   M:	Tomasz Duszynski <tduszyns@gmail.com>
>   S:	Maintained

Looks good to me,

Reviewed-by: Ivan Orlov <ivan.orlov0322@gmail.com>
-- 
Kind regards,
Ivan Orlov

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

[Ivan Orlov]

On 6/3/24 17:21, Mudit Sharma 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
> 
> Signed-off-by: Mudit Sharma <muditsharma.info@gmail.com>
> ---

LGTM,

Reviewed-by: Ivan Orlov <ivan.orlov0322@gmail.com>

-- 
Kind regards,
Ivan Orlov

Re: [PATCH v2 3/3] MAINTAINERS: Add maintainer for ROHM BH1745

[Javier Carrasco]

On 03/06/2024 18:21, Mudit Sharma wrote:
> Add myself as maintainer for ROHM BH1745 colour sensor driver.
> 
> Signed-off-by: Mudit Sharma <muditsharma.info@gmail.com>
> ---
> v1->v2:
> - No changes
> 
>  MAINTAINERS | 7 +++++++
>  1 file changed, 7 insertions(+)
> 
> diff --git a/MAINTAINERS b/MAINTAINERS
> index d6c90161c7bf..945873321fef 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -19407,6 +19407,13 @@ 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:	Documentation/devicetree/bindings/iio/light/rohm,bh1745.yaml
> +F:	drivers/iio/light/bh1745.c
> +
>  ROHM BH1750 AMBIENT LIGHT SENSOR DRIVER
>  M:	Tomasz Duszynski <tduszyns@gmail.com>
>  S:	Maintained

Hi Mudit,

is there any special reason to have a separate patch for this? The
addition to MAINTANERS for new drives is usually included in the patch
that provides the driver itself.

Best regards,
Javier Carrasco

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

[Javier Carrasco]

On 03/06/2024 18:21, Mudit Sharma 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
> 
> Signed-off-by: Mudit Sharma <muditsharma.info@gmail.com>

Hi Mudit,

a few minor comments inline.

> ---
> v1->v2:
> - No changes
> 
>  drivers/iio/light/Kconfig  |  12 +
>  drivers/iio/light/Makefile |   1 +
>  drivers/iio/light/bh1745.c | 879 +++++++++++++++++++++++++++++++++++++
>  3 files changed, 892 insertions(+)
>  create mode 100644 drivers/iio/light/bh1745.c
> 
> 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..a7b660a1bdc8
> --- /dev/null
> +++ b/drivers/iio/light/bh1745.c
> @@ -0,0 +1,879 @@
> +// 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"

Given that this define is only used in one place, using the string
directly is common practice in iio.

> +
> +/* 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_RED,
> +	BH1745_INT_SOURCE_GREEN,
> +	BH1745_INT_SOURCE_BLUE,
> +	BH1745_INT_SOURCE_CLEAR,
> +} bh1745_int_source;
> +
> +enum {
> +	BH1745_ADC_GAIN_1X,
> +	BH1745_ADC_GAIN_2X,
> +	BH1745_ADC_GAIN_16X,
> +} bh1745_gain;
> +
> +enum {
> +	BH1745_MEASUREMENT_TIME_160MS,
> +	BH1745_MEASUREMENT_TIME_320MS,
> +	BH1745_MEASUREMENT_TIME_640MS,
> +	BH1745_MEASUREMENT_TIME_1280MS,
> +	BH1745_MEASUREMENT_TIME_2560MS,
> +	BH1745_MEASUREMENT_TIME_5120MS,
> +} bh1745_measurement_time;
> +
> +enum {
> +	BH1745_PRESISTENCE_UPDATE_TOGGLE,
> +	BH1745_PRESISTENCE_UPDATE_EACH_MEASUREMENT,
> +	BH1745_PRESISTENCE_UPDATE_FOUR_MEASUREMENT,
> +	BH1745_PRESISTENCE_UPDATE_EIGHT_MEASUREMENT,
> +} bh1745_presistence_value;
> +
> +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)
> +{

The initial assignment is unnecessary, as a new assignment is made
immediately. This applies to several declarations of ret in this driver,
but not always (e.g. bh1745_setup_trigger()).

> +	int ret = 0;
> +
> +	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;
> +	}

Nit: black line before return (it applies to several functions in this
driver, but again, not in all of them).

> +	return ret;
> +}
> +
> +static int bh1745_read_value(struct bh1745_data *data, u8 reg, void *value,
> +			     size_t len)
> +{
> +	int ret = 0;
> +
> +	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;
> +	}
> +	return ret;
> +}
> +
> +static ssize_t in_interrupt_source_show(struct device *dev,
> +					struct device_attribute *attr,
> +					char *buf)
> +{
> +	int ret = 0;
> +	int value = 0;
> +	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 = 0;
> +	u16 value = 0;
> +	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;
> +	}

Consider using guard(mutex)(&data->lock) instead, which is becoming
common practice in iio. In principle, that applies to all uses of the mutex.

> +	mutex_lock(&data->lock);
> +	data->int_src = value;
> +	value = value << 2;
> +	ret = bh1745_read_value(data, BH1745_INTR, &data->interrupt, 1);
> +	if (ret < 0) {
> +		mutex_unlock(&data->lock);
> +		return ret;
> +	}
> +	data->interrupt &= ~BH1745_INT_SOURCE_MASK;
> +	data->interrupt |= value;
> +	ret = bh1745_write_value(data, BH1745_INTR, &data->interrupt, 1);
> +	mutex_unlock(&data->lock);
> +	if (ret < 0)
> +		return ret;
> +	return len;
> +}
> +
> +static ssize_t in_interrupt_latch_show(struct device *dev,
> +				       struct device_attribute *attr, char *buf)
> +{
> +	int value = 0;
> +	struct iio_dev *indio_dev = dev_to_iio_dev(dev);
> +	struct bh1745_data *data = iio_priv(indio_dev);
> +
> +	bh1745_read_value(data, BH1745_INTR, &value, 1);
> +
> +	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 = 0;
> +	u16 value = 0;
> +	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 == 0) {
> +		data->int_latch = value;
> +		mutex_lock(&data->lock);
> +		ret = bh1745_read_value(data, BH1745_INTR, &data->interrupt, 1);
> +		if (ret < 0) {
> +			mutex_unlock(&data->lock);
> +			return ret;
> +		}
> +
> +		data->interrupt &= ~BH1745_INT_SIGNAL_LATCHED;
> +		ret = bh1745_write_value(data, BH1745_INTR, &data->interrupt,
> +					 1);
> +		mutex_unlock(&data->lock);
> +		if (ret < 0)
> +			return ret;
> +
> +	} else if (value == 1) {
> +		data->int_latch = value;
> +		mutex_lock(&data->lock);
> +		ret = bh1745_read_value(data, BH1745_INTR, &data->interrupt, 1);
> +		if (ret < 0) {
> +			mutex_unlock(&data->lock);
> +			return ret;
> +		}
> +
> +		data->interrupt |= BH1745_INT_SIGNAL_LATCHED;
> +		ret = bh1745_write_value(data, BH1745_INTR, &data->interrupt,
> +					 1);
> +		mutex_unlock(&data->lock);
> +		if (ret < 0)
> +			return ret;
> +
> +	} else {
> +		dev_err(dev,
> +			"Value out of range for latch setup. Supported values '0' or '1'\n");
> +	}
> +	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,

Nit: alignment should match open parenthesis.

> +						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 = 0;
> +	u8 value = 0;
> +
> +	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 = 0;
> +	u8 value = 0;
> +
> +	ret = bh1745_read_value(data, BH1745_MODE_CTRL_2, &value, 1);
> +	if (ret < 0)
> +		return ret;
> +
> +	value |= BH1745_RGBC_EN;
> +	mutex_lock(&data->lock);
> +	data->mode_ctrl2 = value;
> +	ret = bh1745_write_value(data, BH1745_MODE_CTRL_2, &data->mode_ctrl2,
> +				 1);
> +	mutex_unlock(&data->lock);
> +
> +	return ret;
> +}
> +
> +static int bh1745_power_off(struct bh1745_data *data)
> +{
> +	int ret = 0;
> +	int value = 0;
> +
> +	ret = bh1745_read_value(data, BH1745_MODE_CTRL_2, &value, 1);
> +	if (ret < 0)
> +		return ret;
> +
> +	value &= ~BH1745_RGBC_EN;
> +	mutex_lock(&data->lock);
> +	data->mode_ctrl2 = value;
> +	ret = bh1745_write_value(data, BH1745_MODE_CTRL_2, &data->mode_ctrl2,
> +				 1);
> +	mutex_unlock(&data->lock);
> +	return ret;
> +}
> +
> +static int bh1745_set_int_time(struct bh1745_data *data, int val, int val2)
> +{
> +	int ret = 0;
> +
> +	for (u8 i = 0; i < ARRAY_SIZE(bh1745_int_time); i++) {
> +		if (val == bh1745_int_time[i][0] &&
> +		    val2 == bh1745_int_time[i][1]) {
> +			mutex_lock(&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);
> +			mutex_unlock(&data->lock);
> +			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 = 0;
> +	u16 value = 0;
> +
> +	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: {
> +		mutex_lock(&data->lock);
> +		ret = bh1745_read_value(data, BH1745_MODE_CTRL_2,
> +					&data->mode_ctrl2, 1);
> +		if (ret < 0) {
> +			mutex_unlock(&data->lock);
> +			return ret;
> +		}
> +
> +		value = data->mode_ctrl2 & BH1745_ADC_GAIN_MASK;
> +		mutex_unlock(&data->lock);
> +
> +		*val = bh1745_gain_factor[value];
> +		return IIO_VAL_INT;
> +	}
> +
> +	case IIO_CHAN_INFO_INT_TIME: {
> +		mutex_lock(&data->lock);
> +		ret = bh1745_read_value(data, BH1745_MODE_CTRL_1,
> +					&data->mode_ctrl1, 1);
> +		if (ret < 0) {
> +			mutex_unlock(&data->lock);
> +			return ret;
> +		}
> +		value = data->mode_ctrl1 & BH1745_MEASUREMENT_TIME_MASK;
> +		mutex_unlock(&data->lock);
> +
> +		*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 = 0;
> +
> +	switch (mask) {
> +	case IIO_CHAN_INFO_HARDWAREGAIN: {
> +		for (u8 i = 0; i < ARRAY_SIZE(bh1745_gain_factor); i++) {
> +			if (bh1745_gain_factor[i] == val) {
> +				mutex_lock(&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);
> +				mutex_unlock(&data->lock);
> +				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 = 0;
> +
> +	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 = 0;
> +
> +	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 = 0;
> +	u8 val = 0;
> +	struct iio_dev *indio_dev = iio_trigger_get_drvdata(trig);
> +	struct bh1745_data *data = iio_priv(indio_dev);
> +
> +	if (state) {
> +		mutex_lock(&data->lock);
> +		ret = bh1745_read_value(data, BH1745_INTR, &val, 1);
> +		val |= (BH1745_INT_ENABLE |
> +			(data->int_latch << BH1745_INT_SIGNAL_LATCH_OFFSET) |
> +			(data->int_src << BH1745_INT_SOURCE_OFFSET));
> +		data->interrupt = val;
> +		ret = bh1745_write_value(data, BH1745_INTR, &data->interrupt,
> +					 1);
> +		mutex_unlock(&data->lock);
> +	} else {
> +		mutex_lock(&data->lock);
> +		data->interrupt = val;
> +		ret = bh1745_write_value(data, BH1745_INTR, &data->interrupt,
> +					 1);
> +		mutex_unlock(&data->lock);
> +	}
> +	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 = 0;
> +	int value = 0;
> +
> +	ret = bh1745_read_value(data, BH1745_INTR, &value, 1);
> +	if (ret < 0)
> +		return ret;
> +
> +	if (value & BH1745_INTR_STATUS) {
> +		mutex_lock(&data->lock);
> +		iio_push_event(indio_dev,

Nit: lines should not end with an open parenthesis. You can simply move
IIO_INTENSITY up.

> +			       IIO_UNMOD_EVENT_CODE(
> +				       IIO_INTENSITY, data->int_src,
> +				       IIO_EV_TYPE_THRESH, IIO_EV_DIR_EITHER),
> +			       iio_get_time_ns(indio_dev));
> +
> +		mutex_unlock(&data->lock);
> +		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);
> +	u16 value = 0;
> +	struct {
> +		u16 chans[4];

Intended blank line?

> +
> +		s64 timestamp __aligned(8);
> +	} scan;
> +
> +	int i, j = 0;
> +
> +	for_each_set_bit(i, indio_dev->active_scan_mask,
> +			 indio_dev->masklength) {
> +		bh1745_read_value(data, BH1745_R_LSB + 2 * i, &value, 2);
> +		scan.chans[j++] = value;
> +	}
> +
> +	iio_push_to_buffers_with_timestamp(indio_dev, &scan,
> +					   iio_get_time_ns(indio_dev));
> +
> +	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);

The rest of your "if (ret)" don't use curly braces for a return
dev_err_probe().

> +	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 = 0;
> +
> +	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 = 0;
> +	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_MOD_NAME;
> +	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");
> +
> +	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");


Best regards,
Javier Carrasco

Re: [PATCH v2 1/3] dt-bindings: iio: light: ROHM BH1745

[Krzysztof Kozlowski]

On 03/06/2024 18:21, Mudit Sharma wrote:
> Add ROHM BH1745 - 4 channel I2C colour sensor's dt-bindings.
> 
> Signed-off-by: Mudit Sharma <muditsharma.info@gmail.com>
> ---
> v1->v2:
> - Fix yaml issue: Make `maintainers` a list

Judging by driver there will be v3, so few nits.

> 
>  .../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..ac5c4d160513
> --- /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: |

Do not need '|' unless you need to preserve formatting.

> +  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
> +
> +additionalProperties: false

Please put this after required: block.

> +
> +required:
> +  - compatible
> +  - reg
> +


Reviewed-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>


---

This is an automated instruction, just in case, because many review tags
are being ignored. If you know the process, you can skip it (please do
not feel offended by me posting it here - no bad intentions intended).
If you do not know the process, here is a short explanation:

Please add Acked-by/Reviewed-by/Tested-by tags when posting new
versions, under or above your Signed-off-by tag. Tag is "received", when
provided in a message replied to you on the mailing list. Tools like b4
can help here. However, there's no need to repost patches *only* to add
the tags. The upstream maintainer will do that for tags received on the
version they apply.

https://elixir.bootlin.com/linux/v6.5-rc3/source/Documentation/process/submitting-patches.rst#L577

Best regards,
Krzysztof

Re: [PATCH v2 3/3] MAINTAINERS: Add maintainer for ROHM BH1745

[Mudit Sharma]

On 03/06/2024 23:37, Javier Carrasco wrote:
> On 03/06/2024 18:21, Mudit Sharma wrote:
>> Add myself as maintainer for ROHM BH1745 colour sensor driver.
>>
>> Signed-off-by: Mudit Sharma <muditsharma.info@gmail.com>
>> ---
>> v1->v2:
>> - No changes
>>
>>   MAINTAINERS | 7 +++++++
>>   1 file changed, 7 insertions(+)
>>
>> diff --git a/MAINTAINERS b/MAINTAINERS
>> index d6c90161c7bf..945873321fef 100644
>> --- a/MAINTAINERS
>> +++ b/MAINTAINERS
>> @@ -19407,6 +19407,13 @@ 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:	Documentation/devicetree/bindings/iio/light/rohm,bh1745.yaml
>> +F:	drivers/iio/light/bh1745.c
>> +
>>   ROHM BH1750 AMBIENT LIGHT SENSOR DRIVER
>>   M:	Tomasz Duszynski <tduszyns@gmail.com>
>>   S:	Maintained
> 
> Hi Mudit,
> 
> is there any special reason to have a separate patch for this? The
> addition to MAINTANERS for new drives is usually included in the patch
> that provides the driver itself.
> 
> Best regards,
> Javier Carrasco

Hi Javier,

Adding this in a separate commit was just a pattern I notices with some
other drivers, for instance 3b4e0e9.

If necessary and/or considered good practice, I can squash this in the 
patch that brings in the driver.

Best regards,
Mudit Sharma

Re: [PATCH v2 3/3] MAINTAINERS: Add maintainer for ROHM BH1745

[Javier Carrasco]

On 04/06/2024 12:44, Mudit Sharma wrote:
> On 03/06/2024 23:37, Javier Carrasco wrote:
>> On 03/06/2024 18:21, Mudit Sharma wrote:
>>> Add myself as maintainer for ROHM BH1745 colour sensor driver.
>>>
>>> Signed-off-by: Mudit Sharma <muditsharma.info@gmail.com>
>>> ---
>>> v1->v2:
>>> - No changes
>>>
>>>   MAINTAINERS | 7 +++++++
>>>   1 file changed, 7 insertions(+)
>>>
>>> diff --git a/MAINTAINERS b/MAINTAINERS
>>> index d6c90161c7bf..945873321fef 100644
>>> --- a/MAINTAINERS
>>> +++ b/MAINTAINERS
>>> @@ -19407,6 +19407,13 @@ 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:    Documentation/devicetree/bindings/iio/light/rohm,bh1745.yaml
>>> +F:    drivers/iio/light/bh1745.c
>>> +
>>>   ROHM BH1750 AMBIENT LIGHT SENSOR DRIVER
>>>   M:    Tomasz Duszynski <tduszyns@gmail.com>
>>>   S:    Maintained
>>
>> Hi Mudit,
>>
>> is there any special reason to have a separate patch for this? The
>> addition to MAINTANERS for new drives is usually included in the patch
>> that provides the driver itself.
>>
>> Best regards,
>> Javier Carrasco
> 
> Hi Javier,
> 
> Adding this in a separate commit was just a pattern I notices with some
> other drivers, for instance 3b4e0e9.
> 
> If necessary and/or considered good practice, I can squash this in the
> patch that brings in the driver.
> 
> Best regards,
> Mudit Sharma

Although there might be some cases where it was added separately, it is
much more common that it is added to the patch that provides the driver.
Some perfectionists even include the entry in the dt-bindings patch, and
then add the link to the driver code in the driver patch. I believe that
a simple squash would be ok, though.

Best regards,
Javier Carrasco

Re: [PATCH v2 3/3] MAINTAINERS: Add maintainer for ROHM BH1745

[Matti Vaittinen]

On 6/4/24 15:53, Javier Carrasco wrote:
> On 04/06/2024 12:44, Mudit Sharma wrote:
>> On 03/06/2024 23:37, Javier Carrasco wrote:
>>> On 03/06/2024 18:21, Mudit Sharma wrote:
>>>> Add myself as maintainer for ROHM BH1745 colour sensor driver.
>>>>
>>>
>>> is there any special reason to have a separate patch for this? The
>>> addition to MAINTANERS for new drives is usually included in the patch
>>> that provides the driver itself.
>>
>> Adding this in a separate commit was just a pattern I notices with some
>> other drivers, for instance 3b4e0e9.
>>
>> If necessary and/or considered good practice, I can squash this in the
>> patch that brings in the driver.
> 
> Although there might be some cases where it was added separately, it is
> much more common that it is added to the patch that provides the driver.
> Some perfectionists even include the entry in the dt-bindings patch, and
> then add the link to the driver code in the driver patch. I believe that
> a simple squash would be ok, though.

I believe there is a notable case where having MAINTAINERS updates as a 
separate patch makes sense. When one creates drivers for a device which 
touches multiple subsystems, typically a set of MFD drivers. This is 
usually done as a set of subsystem specific patches, each adding 
subsystem specific file(s). In this case adding MAINTAINER info 
separately for each sub-driver will create unnecessary churn in the 
MAINTAINERS file - which I believe is already now a major source of 
merge conflicts. I am not sure of this is a reason to have MAINTAINERS 
updates in own patch though.

Furthermore, I've been instructed by Rob (AFAIR) to omit the dt-binding 
files from the MAINTAINERS because the maintainer information is already 
contained in the bindings itself.

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 v2 3/3] MAINTAINERS: Add maintainer for ROHM BH1745

[Javier Carrasco]

On 04/06/2024 15:38, Matti Vaittinen wrote:
> On 6/4/24 15:53, Javier Carrasco wrote:
>> On 04/06/2024 12:44, Mudit Sharma wrote:
>>> On 03/06/2024 23:37, Javier Carrasco wrote:
>>>> On 03/06/2024 18:21, Mudit Sharma wrote:
>>>>> Add myself as maintainer for ROHM BH1745 colour sensor driver.
>>>>>
>>>>
>>>> is there any special reason to have a separate patch for this? The
>>>> addition to MAINTANERS for new drives is usually included in the patch
>>>> that provides the driver itself.
>>>
>>> Adding this in a separate commit was just a pattern I notices with some
>>> other drivers, for instance 3b4e0e9.
>>>
>>> If necessary and/or considered good practice, I can squash this in the
>>> patch that brings in the driver.
>>
>> Although there might be some cases where it was added separately, it is
>> much more common that it is added to the patch that provides the driver.
>> Some perfectionists even include the entry in the dt-bindings patch, and
>> then add the link to the driver code in the driver patch. I believe that
>> a simple squash would be ok, though.
> 
> I believe there is a notable case where having MAINTAINERS updates as a
> separate patch makes sense. When one creates drivers for a device which
> touches multiple subsystems, typically a set of MFD drivers. This is
> usually done as a set of subsystem specific patches, each adding
> subsystem specific file(s). In this case adding MAINTAINER info
> separately for each sub-driver will create unnecessary churn in the
> MAINTAINERS file - which I believe is already now a major source of
> merge conflicts. I am not sure of this is a reason to have MAINTAINERS
> updates in own patch though.
> 
> Furthermore, I've been instructed by Rob (AFAIR) to omit the dt-binding
> files from the MAINTAINERS because the maintainer information is already
> contained in the bindings itself.
> 
> Yours,
>     -- Matti
> 

Good point, in that case it would make more sense. But this driver only
apllies to iio, and there won't be such conflicts. But thank you for
that clarification.

By the way, I think you have some issue with your email configuration
(no line wrapping).

Best regards,
Javier Carrasco

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

[Mudit Sharma]

On 04/06/2024 00:10, Javier Carrasco wrote:
> On 03/06/2024 18:21, Mudit Sharma 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
>>
>> Signed-off-by: Mudit Sharma <muditsharma.info@gmail.com>
> 
> Hi Mudit,
> 
> a few minor comments inline.
> 
>> ---
>> v1->v2:
>> - No changes
>>
>>   drivers/iio/light/Kconfig  |  12 +
>>   drivers/iio/light/Makefile |   1 +
>>   drivers/iio/light/bh1745.c | 879 +++++++++++++++++++++++++++++++++++++
>>   3 files changed, 892 insertions(+)
>>   create mode 100644 drivers/iio/light/bh1745.c
>>
>> 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..a7b660a1bdc8
>> --- /dev/null
>> +++ b/drivers/iio/light/bh1745.c
>> @@ -0,0 +1,879 @@
>> +// 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"
> 
> Given that this define is only used in one place, using the string
> directly is common practice in iio.
> 
>> +
>> +/* 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_RED,
>> +	BH1745_INT_SOURCE_GREEN,
>> +	BH1745_INT_SOURCE_BLUE,
>> +	BH1745_INT_SOURCE_CLEAR,
>> +} bh1745_int_source;
>> +
>> +enum {
>> +	BH1745_ADC_GAIN_1X,
>> +	BH1745_ADC_GAIN_2X,
>> +	BH1745_ADC_GAIN_16X,
>> +} bh1745_gain;
>> +
>> +enum {
>> +	BH1745_MEASUREMENT_TIME_160MS,
>> +	BH1745_MEASUREMENT_TIME_320MS,
>> +	BH1745_MEASUREMENT_TIME_640MS,
>> +	BH1745_MEASUREMENT_TIME_1280MS,
>> +	BH1745_MEASUREMENT_TIME_2560MS,
>> +	BH1745_MEASUREMENT_TIME_5120MS,
>> +} bh1745_measurement_time;
>> +
>> +enum {
>> +	BH1745_PRESISTENCE_UPDATE_TOGGLE,
>> +	BH1745_PRESISTENCE_UPDATE_EACH_MEASUREMENT,
>> +	BH1745_PRESISTENCE_UPDATE_FOUR_MEASUREMENT,
>> +	BH1745_PRESISTENCE_UPDATE_EIGHT_MEASUREMENT,
>> +} bh1745_presistence_value;
>> +
>> +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)
>> +{
> 
> The initial assignment is unnecessary, as a new assignment is made
> immediately. This applies to several declarations of ret in this driver,
> but not always (e.g. bh1745_setup_trigger()).
> 
>> +	int ret = 0;
>> +
>> +	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;
>> +	}
> 
> Nit: black line before return (it applies to several functions in this
> driver, but again, not in all of them).

Hi Javier,

Thank you for the review on this.

Can you please point me to resource/section of code style guide for 
reference which talks about new line before 'return'.

Best regards,
Mudit Sharma

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

[Javier Carrasco]

On 04/06/2024 16:24, Mudit Sharma wrote:
> On 04/06/2024 00:10, Javier Carrasco wrote:
>> On 03/06/2024 18:21, Mudit Sharma 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
>>>
>>> Signed-off-by: Mudit Sharma <muditsharma.info@gmail.com>
>>
>> Hi Mudit,
>>
>> a few minor comments inline.
>>
>>> ---
>>> v1->v2:
>>> - No changes
>>>
>>>   drivers/iio/light/Kconfig  |  12 +
>>>   drivers/iio/light/Makefile |   1 +
>>>   drivers/iio/light/bh1745.c | 879 +++++++++++++++++++++++++++++++++++++
>>>   3 files changed, 892 insertions(+)
>>>   create mode 100644 drivers/iio/light/bh1745.c
>>>
>>> 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..a7b660a1bdc8
>>> --- /dev/null
>>> +++ b/drivers/iio/light/bh1745.c
>>> @@ -0,0 +1,879 @@
>>> +// 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"
>>
>> Given that this define is only used in one place, using the string
>> directly is common practice in iio.
>>
>>> +
>>> +/* 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_RED,
>>> +    BH1745_INT_SOURCE_GREEN,
>>> +    BH1745_INT_SOURCE_BLUE,
>>> +    BH1745_INT_SOURCE_CLEAR,
>>> +} bh1745_int_source;
>>> +
>>> +enum {
>>> +    BH1745_ADC_GAIN_1X,
>>> +    BH1745_ADC_GAIN_2X,
>>> +    BH1745_ADC_GAIN_16X,
>>> +} bh1745_gain;
>>> +
>>> +enum {
>>> +    BH1745_MEASUREMENT_TIME_160MS,
>>> +    BH1745_MEASUREMENT_TIME_320MS,
>>> +    BH1745_MEASUREMENT_TIME_640MS,
>>> +    BH1745_MEASUREMENT_TIME_1280MS,
>>> +    BH1745_MEASUREMENT_TIME_2560MS,
>>> +    BH1745_MEASUREMENT_TIME_5120MS,
>>> +} bh1745_measurement_time;
>>> +
>>> +enum {
>>> +    BH1745_PRESISTENCE_UPDATE_TOGGLE,
>>> +    BH1745_PRESISTENCE_UPDATE_EACH_MEASUREMENT,
>>> +    BH1745_PRESISTENCE_UPDATE_FOUR_MEASUREMENT,
>>> +    BH1745_PRESISTENCE_UPDATE_EIGHT_MEASUREMENT,
>>> +} bh1745_presistence_value;
>>> +
>>> +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)
>>> +{
>>
>> The initial assignment is unnecessary, as a new assignment is made
>> immediately. This applies to several declarations of ret in this driver,
>> but not always (e.g. bh1745_setup_trigger()).
>>
>>> +    int ret = 0;
>>> +
>>> +    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;
>>> +    }
>>
>> Nit: black line before return (it applies to several functions in this
>> driver, but again, not in all of them).
> 
> Hi Javier,
> 
> Thank you for the review on this.
> 
> Can you please point me to resource/section of code style guide for
> reference which talks about new line before 'return'.
> 
> Best regards,
> Mudit Sharma
> 
> 
> 

AFAIK that is not written in stone, but many common practices are not
documented anywhere (e.g. names of error/ret variables). They just copy
what the majority of the code in that subsystem does. There is indeed a
tendency to add a blank line before the last (unconditional, not
labeled) return, but I am sure that some code does not follow that.

Having said that, I don't have a strong opinion (it was a nitpick) on
that, but what I would definitely recommend you is following **some**
pattern. There are some functions where you added a blank line, and some
others (the majority, I think), where you didn't. Given that this is new
code, uniformity would be appreciated.

Unless an IIO maintainer (I am NOT one) says otherwise, I would find
both approaches (blank/no line) reasonable, even though I like the blank
line in that particular case :)

Best regards,
Javier Carrasco

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

[Mudit Sharma]

On 04/06/2024 15:58, Javier Carrasco wrote:
> On 04/06/2024 16:24, Mudit Sharma wrote:
>> On 04/06/2024 00:10, Javier Carrasco wrote:
>>> On 03/06/2024 18:21, Mudit Sharma 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
>>>>
>>>> Signed-off-by: Mudit Sharma <muditsharma.info@gmail.com>
>>>
>>> Hi Mudit,
>>>
>>> a few minor comments inline.
>>>
>>>> ---
>>>> v1->v2:
>>>> - No changes
>>>>
>>>>    drivers/iio/light/Kconfig  |  12 +
>>>>    drivers/iio/light/Makefile |   1 +
>>>>    drivers/iio/light/bh1745.c | 879 +++++++++++++++++++++++++++++++++++++
>>>>    3 files changed, 892 insertions(+)
>>>>    create mode 100644 drivers/iio/light/bh1745.c
>>>>
>>>> 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..a7b660a1bdc8
>>>> --- /dev/null
>>>> +++ b/drivers/iio/light/bh1745.c
>>>> @@ -0,0 +1,879 @@
>>>> +// 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"
>>>
>>> Given that this define is only used in one place, using the string
>>> directly is common practice in iio.
>>>
>>>> +
>>>> +/* 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_RED,
>>>> +    BH1745_INT_SOURCE_GREEN,
>>>> +    BH1745_INT_SOURCE_BLUE,
>>>> +    BH1745_INT_SOURCE_CLEAR,
>>>> +} bh1745_int_source;
>>>> +
>>>> +enum {
>>>> +    BH1745_ADC_GAIN_1X,
>>>> +    BH1745_ADC_GAIN_2X,
>>>> +    BH1745_ADC_GAIN_16X,
>>>> +} bh1745_gain;
>>>> +
>>>> +enum {
>>>> +    BH1745_MEASUREMENT_TIME_160MS,
>>>> +    BH1745_MEASUREMENT_TIME_320MS,
>>>> +    BH1745_MEASUREMENT_TIME_640MS,
>>>> +    BH1745_MEASUREMENT_TIME_1280MS,
>>>> +    BH1745_MEASUREMENT_TIME_2560MS,
>>>> +    BH1745_MEASUREMENT_TIME_5120MS,
>>>> +} bh1745_measurement_time;
>>>> +
>>>> +enum {
>>>> +    BH1745_PRESISTENCE_UPDATE_TOGGLE,
>>>> +    BH1745_PRESISTENCE_UPDATE_EACH_MEASUREMENT,
>>>> +    BH1745_PRESISTENCE_UPDATE_FOUR_MEASUREMENT,
>>>> +    BH1745_PRESISTENCE_UPDATE_EIGHT_MEASUREMENT,
>>>> +} bh1745_presistence_value;
>>>> +
>>>> +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)
>>>> +{
>>>
>>> The initial assignment is unnecessary, as a new assignment is made
>>> immediately. This applies to several declarations of ret in this driver,
>>> but not always (e.g. bh1745_setup_trigger()).
>>>
>>>> +    int ret = 0;
>>>> +
>>>> +    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;
>>>> +    }
>>>
>>> Nit: black line before return (it applies to several functions in this
>>> driver, but again, not in all of them).
>>
>> Hi Javier,
>>
>> Thank you for the review on this.
>>
>> Can you please point me to resource/section of code style guide for
>> reference which talks about new line before 'return'.
>>
>> Best regards,
>> Mudit Sharma
>>
>>
>>
> 
> AFAIK that is not written in stone, but many common practices are not
> documented anywhere (e.g. names of error/ret variables). They just copy
> what the majority of the code in that subsystem does. There is indeed a
> tendency to add a blank line before the last (unconditional, not
> labeled) return, but I am sure that some code does not follow that.
> 
> Having said that, I don't have a strong opinion (it was a nitpick) on
> that, but what I would definitely recommend you is following **some**
> pattern. There are some functions where you added a blank line, and some
> others (the majority, I think), where you didn't. Given that this is new
> code, uniformity would be appreciated.
> 
> Unless an IIO maintainer (I am NOT one) says otherwise, I would find
> both approaches (blank/no line) reasonable, even though I like the blank
> line in that particular case :)
> 
> Best regards,
> Javier Carrasco

Thanks for the explanation here.

I agree with having a consistent pattern and will make the necessary 
changes to v3.

Best regards,
Mudit Sharma

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

[kernel test robot]

Hi Mudit,

kernel test robot noticed the following build warnings:

[auto build test WARNING on jic23-iio/togreg]
[also build test WARNING on linus/master v6.10-rc2 next-20240605]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Mudit-Sharma/iio-light-ROHM-BH1745-colour-sensor/20240604-002405
base:   https://git.kernel.org/pub/scm/linux/kernel/git/jic23/iio.git togreg
patch link:    https://lore.kernel.org/r/20240603162122.165943-2-muditsharma.info%40gmail.com
patch subject: [PATCH v2 2/3] iio: light: ROHM BH1745 colour sensor
config: csky-randconfig-r112-20240605 (https://download.01.org/0day-ci/archive/20240605/202406051506.orRmnC3s-lkp@intel.com/config)
compiler: csky-linux-gcc (GCC) 13.2.0
reproduce: (https://download.01.org/0day-ci/archive/20240605/202406051506.orRmnC3s-lkp@intel.com/reproduce)

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

sparse warnings: (new ones prefixed by >>)
>> drivers/iio/light/bh1745.c:99:3: sparse: sparse: symbol 'bh1745_int_source' was not declared. Should it be static?
>> drivers/iio/light/bh1745.c:105:3: sparse: sparse: symbol 'bh1745_gain' was not declared. Should it be static?
>> drivers/iio/light/bh1745.c:114:3: sparse: sparse: symbol 'bh1745_measurement_time' was not declared. Should it be static?
>> drivers/iio/light/bh1745.c:121:3: sparse: sparse: symbol 'bh1745_presistence_value' was not declared. Should it be static?

vim +/bh1745_int_source +99 drivers/iio/light/bh1745.c

    93	
    94	enum {
    95		BH1745_INT_SOURCE_RED,
    96		BH1745_INT_SOURCE_GREEN,
    97		BH1745_INT_SOURCE_BLUE,
    98		BH1745_INT_SOURCE_CLEAR,
  > 99	} bh1745_int_source;
   100	
   101	enum {
   102		BH1745_ADC_GAIN_1X,
   103		BH1745_ADC_GAIN_2X,
   104		BH1745_ADC_GAIN_16X,
 > 105	} bh1745_gain;
   106	
   107	enum {
   108		BH1745_MEASUREMENT_TIME_160MS,
   109		BH1745_MEASUREMENT_TIME_320MS,
   110		BH1745_MEASUREMENT_TIME_640MS,
   111		BH1745_MEASUREMENT_TIME_1280MS,
   112		BH1745_MEASUREMENT_TIME_2560MS,
   113		BH1745_MEASUREMENT_TIME_5120MS,
 > 114	} bh1745_measurement_time;
   115	
   116	enum {
   117		BH1745_PRESISTENCE_UPDATE_TOGGLE,
   118		BH1745_PRESISTENCE_UPDATE_EACH_MEASUREMENT,
   119		BH1745_PRESISTENCE_UPDATE_FOUR_MEASUREMENT,
   120		BH1745_PRESISTENCE_UPDATE_EIGHT_MEASUREMENT,
 > 121	} bh1745_presistence_value;
   122	

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

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

[Jonathan Cameron]

> >>>
> >>> Nit: black line before return (it applies to several functions in this
> >>> driver, but again, not in all of them).  
> >>
> >> Hi Javier,
> >>
> >> Thank you for the review on this.
> >>
> >> Can you please point me to resource/section of code style guide for
> >> reference which talks about new line before 'return'.
> >>
> >> Best regards,
> >> Mudit Sharma
> >>
> >>
> >>  
> > 
> > AFAIK that is not written in stone, but many common practices are not
> > documented anywhere (e.g. names of error/ret variables). They just copy
> > what the majority of the code in that subsystem does. There is indeed a
> > tendency to add a blank line before the last (unconditional, not
> > labeled) return, but I am sure that some code does not follow that.
> > 
> > Having said that, I don't have a strong opinion (it was a nitpick) on
> > that, but what I would definitely recommend you is following **some**
> > pattern. There are some functions where you added a blank line, and some
> > others (the majority, I think), where you didn't. Given that this is new
> > code, uniformity would be appreciated.
> > 
> > Unless an IIO maintainer (I am NOT one) says otherwise, I would find
> > both approaches (blank/no line) reasonable, even though I like the blank
> > line in that particular case :)
> > 
> > Best regards,
> > Javier Carrasco  
> 
> Thanks for the explanation here.
> 
> I agree with having a consistent pattern and will make the necessary 
> changes to v3.
> 
> Best regards,
> Mudit Sharma
> 
I'm feeling grumpy today and you are the unlucky ones, given it's
been a day of much scrolling.

Crop your replies to just the relevant context as I've done here.

Yes, I prefer the blank line in most cases. However as noted the more
important factor is local consistency.  Aim here is to make your code
as easy to review as possible - having it all look the same is a good
way to help that.

Jonathan