Re: [PATCH v2 2/3] iio: light: ROHM BH1745 colour sensor
From: Mudit Sharma
Date: Tue Jun 04 2024 - 10:25:00 EST
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@xxxxxxxxx>
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@xxxxxxxxx>
+ *
+ * 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