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).
> + 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@xxxxxxxxx>");
> +MODULE_DESCRIPTION("BH1745 colour sensor driver");
> +MODULE_LICENSE("GPL");
Best regards,
Javier Carrasco
