Add support for ROHM BU27008 RGB sensor.
The ROHM BU27008 is a sensor with 5 photodiodes (red, green, blue, clear
and IR) with four configurable channels. Red and green being always
available and two out of the rest three (blue, clear, IR) can be
selected to be simultaneously measured. Typical application is adjusting
LCD backlight of TVs, mobile phones and tablet PCs.
This series supports reading the RGBC and IR channels using IIO
frameeork. However, only two of the BC+IR can be enabled at the same
time. Series adds also support for scale and integration time
configuration, where scale consists of impact of both the integration
time and hardware gain. The gain and time support is backed by the newly
introduced IIO GTS helper. This series depends on GTS helper patches
added in BU27034 support series which is already merged in iio/togreg
which this series is based on.
The hardware allows configuring gain setting by writing a 5-bit gain
selector value to a register. Part of the gain setting is common for all
channels (RGBC + IR) but part of the selector value can be set
separately for RGBC and IR:
MODE_CONTROL2 REG:
bit 7 6 5 4 3 2 1 0
-----------------------------------------------------------------
| RGB selector |
+---------------------------------------+
-----------------------------------------------------------------
| high bits IR | | low bits IR selector |
+---------------+ +-----------------------+
In theory it would be possible to set certain separate gain values for
RGBC and IR channels, but this gets pretty confusing because there are a
few 'unsupported' selector values. If only RGBC or IR was set, some
extra handling should be done to prevent the other channel from getting
unsupported value due to change in high-bits. Furthermore, allowing the
channels to be set different gain values (in some cases when gains are
such the HW supports it) would make the cases where also integration
time is changed to achieve correct scale ... interesting. It might also
be confusing for user to try predicting when setting different scales
succeeds and when it does not. Furthermore, if for example the scale
setting for RGBC caused IR selector to be invalid - it could also cause
the IR scale to "jump" very far from previous value.
To make the code simpler and more predictable for users, the current
logic is as follows:
1. Prevent setting IR scale. (My assumption is IR is less used than
RGBC)
2. When RGBC scale is set, set also the IR-selector to the same value.
This prevents unsupported selector values and makes the IR scale changes
predictable.
The 2) could mean we effectively have the same scale for all channels.
Unfortunately, the HW design is slightly peculiar and selector 0 means
gain 1X on RGBC but gain 2X on IR. Rest of the selectors equal same gain
values on RGBC and IR. The result is that while changin selector from 0
=> 1 causes RGBC gain to go from 1X => 4X, it causes IR gain to go from
2X => 4X.
So, the driver provides separate scale entries for all channels (also
RGB and C will have separate gain entries because these channels are of
same type as IR channel). This makes it possible for user applications
to go read the scales for all channels after setting scale for one (in
order to detect the IR scale difference).
Having the separate IR scale entry which applications can read to detect
"arbitrary scale changes" makes it possible for applications to be
written so they can cope if we need to implement the 'allow setting some
different gains for IR and RGBC' - later.
Finally, the scales_available is also provided for all other channels
except the IR channel, which does not allow the scale to be changed.
The sensor provides a data-ready IRQ and the driver implements a
triggered buffer mode using this IRQ as a trigger.
---
Matti Vaittinen (3):
dt-bindings: iio: light: ROHM BU27008
iio: light: ROHM BU27008 color sensor
MAINTAINERS: Add ROHM BU27008
.../bindings/iio/light/rohm-bu27008.yaml | 49 +
MAINTAINERS | 3 +-
drivers/iio/light/Kconfig | 14 +
drivers/iio/light/Makefile | 1 +
drivers/iio/light/rohm-bu27008.c | 1028 +++++++++++++++++
5 files changed, 1094 insertions(+), 1 deletion(-)
create mode 100644 Documentation/devicetree/bindings/iio/light/rohm-bu27008.yaml
create mode 100644 drivers/iio/light/rohm-bu27008.c
base-commit: 52cc189b4fc6af6accc45fe7b7053d76d8724059
--
2.40.0
--
Matti Vaittinen, Linux device drivers
ROHM Semiconductors, Finland SWDC
Kiviharjunlenkki 1E
90220 OULU
FINLAND
~~~ "I don't think so," said Rene Descartes. Just then he vanished ~~~
Simon says - in Latin please.
~~~ "non cogito me" dixit Rene Descarte, deinde evanescavit ~~~
Thanks to Simon Glass for the translation =]
The ROHM BU27008 is a sensor with 5 photodiodes (red, green, blue, clear
and IR) with four configurable channels. Red and green being always
available and two out of the rest three (blue, clear, IR) can be
selected to be simultaneously measured. Typical application is adjusting
LCD backlight of TVs, mobile phones and tablet PCs.
Add initial support for the ROHM BU27008 color sensor.
- raw_read() of RGB and clear channels
- triggered buffer w/ DRDY interrtupt
Signed-off-by: Matti Vaittinen <[email protected]>
---
drivers/iio/light/Kconfig | 14 +
drivers/iio/light/Makefile | 1 +
drivers/iio/light/rohm-bu27008.c | 1028 ++++++++++++++++++++++++++++++
3 files changed, 1043 insertions(+)
create mode 100644 drivers/iio/light/rohm-bu27008.c
diff --git a/drivers/iio/light/Kconfig b/drivers/iio/light/Kconfig
index 6fa31fcd71a1..7888fc439b2f 100644
--- a/drivers/iio/light/Kconfig
+++ b/drivers/iio/light/Kconfig
@@ -289,6 +289,20 @@ config JSA1212
To compile this driver as a module, choose M here:
the module will be called jsa1212.
+config ROHM_BU27008
+ tristate "ROHM BU27008 color (RGB+C/IR) sensor"
+ depends on I2C
+ select REGMAP_I2C
+ select IIO_GTS_HELPER
+ help
+ Enable support for the ROHM BU27008 color sensor.
+ The ROHM BU27008 is a sensor with 5 photodiodes (red, green,
+ blue, clear and IR) with four configurable channels. Red and
+ green being always available and two out of the rest three
+ (blue, clear, IR) can be selected to be simultaneously measured.
+ Typical application is adjusting LCD backlight of TVs,
+ mobile phones and tablet PCs.
+
config ROHM_BU27034
tristate "ROHM BU27034 ambient light sensor"
depends on I2C
diff --git a/drivers/iio/light/Makefile b/drivers/iio/light/Makefile
index 985f6feaccd4..881173952301 100644
--- a/drivers/iio/light/Makefile
+++ b/drivers/iio/light/Makefile
@@ -39,6 +39,7 @@ obj-$(CONFIG_NOA1305) += noa1305.o
obj-$(CONFIG_OPT3001) += opt3001.o
obj-$(CONFIG_PA12203001) += pa12203001.o
obj-$(CONFIG_ROHM_BU27034) += rohm-bu27034.o
+obj-$(CONFIG_ROHM_BU27008) += rohm-bu27008.o
obj-$(CONFIG_RPR0521) += rpr0521.o
obj-$(CONFIG_SI1133) += si1133.o
obj-$(CONFIG_SI1145) += si1145.o
diff --git a/drivers/iio/light/rohm-bu27008.c b/drivers/iio/light/rohm-bu27008.c
new file mode 100644
index 000000000000..6fca193eeb9e
--- /dev/null
+++ b/drivers/iio/light/rohm-bu27008.c
@@ -0,0 +1,1028 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * BU27008 ROHM Colour Sensor
+ *
+ * Copyright (c) 2023, ROHM Semiconductor.
+ */
+
+#include <linux/bits.h>
+#include <linux/bitfield.h>
+#include <linux/device.h>
+#include <linux/i2c.h>
+#include <linux/interrupt.h>
+#include <linux/module.h>
+#include <linux/property.h>
+#include <linux/regmap.h>
+#include <linux/regulator/consumer.h>
+#include <linux/units.h>
+
+#include <linux/iio/iio.h>
+#include <linux/iio/iio-gts-helper.h>
+#include <linux/iio/trigger.h>
+#include <linux/iio/trigger_consumer.h>
+#include <linux/iio/triggered_buffer.h>
+
+#define BU27008_REG_SYSTEM_CONTROL 0x40
+#define BU27008_MASK_SW_RESET BIT(7)
+#define BU27008_MASK_PART_ID GENMASK(5, 0)
+#define BU27008_ID 0x1a
+#define BU27008_REG_MODE_CONTROL1 0x41
+#define BU27008_MASK_MEAS_MODE GENMASK(2, 0)
+#define BU27008_MASK_CHAN_SEL GENMASK(3, 2)
+
+#define BU27008_BLUE2_CLEAR3 0x0
+#define BU27008_CLEAR2_IR3 0x1
+#define BU27008_BLUE2_IR3 0x2
+
+#define BU27008_REG_MODE_CONTROL2 0x42
+#define BU27008_MASK_RGBC_GAIN GENMASK(7, 3)
+#define BU27008_MASK_IR_GAIN_LO GENMASK(2, 0)
+#define BU27008_SHIFT_IR_GAIN 3
+
+#define BU27008_REG_MODE_CONTROL3 0x43
+#define BU27008_MASK_VALID BIT(7)
+#define BU27008_MASK_INT_EN BIT(1)
+#define BU27008_MASK_MEAS_EN BIT(0)
+
+#define BU27008_REG_DATA0_LO 0x50
+#define BU27008_REG_DATA1_LO 0x52
+#define BU27008_REG_DATA2_LO 0x54
+#define BU27008_REG_DATA3_LO 0x56
+#define BU27008_REG_DATA3_HI 0x57
+#define BU27008_REG_MANUFACTURER_ID 0x92
+#define BU27008_REG_MAX BU27008_REG_MANUFACTURER_ID
+
+enum {
+ BU27008_RED, /* Always data0 */
+ BU27008_GREEN, /* Always data1 */
+ BU27008_BLUE, /* data2, configurable (blue / clear) */
+ BU27008_CLEAR, /* data2 or data3 */
+ BU27008_IR, /* data3 */
+ BU27008_NUM_CHANS
+};
+
+enum {
+ BU27008_DATA0, /* Always RED */
+ BU27008_DATA1, /* Always GREEN */
+ BU27008_DATA2, /* Blue or Clear */
+ BU27008_DATA3, /* IR or Clear */
+ BU27008_NUM_HW_CHANS
+};
+
+/* We can always measure red and green at same time */
+#define ALWAYS_SCANNABLE (BIT(BU27008_RED) | BIT(BU27008_GREEN))
+
+#define BU27008_CHAN_DATA_SIZE 2 /* Each channel has 16bits of data */
+#define BU27008_BUF_DATA_SIZE (BU27008_NUM_CHANS * BU27008_CHAN_DATA_SIZE)
+#define BU27008_HW_DATA_SIZE (BU27008_NUM_HW_CHANS * BU27008_CHAN_DATA_SIZE)
+#define NUM_U16_IN_TSTAMP (sizeof(s64) / sizeof(u16))
+
+static const unsigned long bu27008_scan_masks[] = {
+ ALWAYS_SCANNABLE | BIT(BU27008_CLEAR) | BIT(BU27008_IR),
+ ALWAYS_SCANNABLE | BIT(BU27008_CLEAR) | BIT(BU27008_BLUE),
+ ALWAYS_SCANNABLE | BIT(BU27008_BLUE) | BIT(BU27008_IR),
+ 0
+};
+
+/*
+ * Available scales with gain 1x - 1024x, timings 55, 100, 200, 400 mS
+ * Time impacts to gain: 1x, 2x, 4x, 8x.
+ *
+ * => Max total gain is HWGAIN * gain by integration time (8 * 1024) = 8192
+ *
+ * Max amplification is (HWGAIN * MAX integration-time multiplier) 1024 * 8
+ * = 8192. With NANO scale we get rid of accuracy loss when we start with the
+ * scale 16.0 for HWGAIN1, INT-TIME 55 mS. This way the nano scale for MAX
+ * total gain 8192 will be 1953125
+ */
+#define BU27008_SCALE_1X 16
+
+/* See the data sheet for the "Gain Setting" table */
+#define BU27008_GSEL_1X 0x00
+#define BU27008_GSEL_4X 0x08
+#define BU27008_GSEL_8X 0x09
+#define BU27008_GSEL_16X 0x0a
+#define BU27008_GSEL_32X 0x0b
+#define BU27008_GSEL_64X 0x0c
+#define BU27008_GSEL_256X 0x18
+#define BU27008_GSEL_512X 0x19
+#define BU27008_GSEL_1024X 0x1a
+
+static const struct iio_gain_sel_pair bu27008_gains[] = {
+ GAIN_SCALE_GAIN(1, BU27008_GSEL_1X),
+ GAIN_SCALE_GAIN(4, BU27008_GSEL_4X),
+ GAIN_SCALE_GAIN(8, BU27008_GSEL_8X),
+ GAIN_SCALE_GAIN(16, BU27008_GSEL_16X),
+ GAIN_SCALE_GAIN(32, BU27008_GSEL_32X),
+ GAIN_SCALE_GAIN(64, BU27008_GSEL_64X),
+ GAIN_SCALE_GAIN(256, BU27008_GSEL_256X),
+ GAIN_SCALE_GAIN(512, BU27008_GSEL_512X),
+ GAIN_SCALE_GAIN(1024, BU27008_GSEL_1024X),
+};
+
+static const struct iio_gain_sel_pair bu27008_gains_ir[] = {
+ GAIN_SCALE_GAIN(2, BU27008_GSEL_1X),
+ GAIN_SCALE_GAIN(4, BU27008_GSEL_4X),
+ GAIN_SCALE_GAIN(8, BU27008_GSEL_8X),
+ GAIN_SCALE_GAIN(16, BU27008_GSEL_16X),
+ GAIN_SCALE_GAIN(32, BU27008_GSEL_32X),
+ GAIN_SCALE_GAIN(64, BU27008_GSEL_64X),
+ GAIN_SCALE_GAIN(256, BU27008_GSEL_256X),
+ GAIN_SCALE_GAIN(512, BU27008_GSEL_512X),
+ GAIN_SCALE_GAIN(1024, BU27008_GSEL_1024X),
+};
+
+#define BU27008_MEAS_MODE_100MS 0x00
+#define BU27008_MEAS_MODE_55MS 0x01
+#define BU27008_MEAS_MODE_200MS 0x02
+#define BU27008_MEAS_MODE_400MS 0x04
+
+static const struct iio_itime_sel_mul bu27008_itimes[] = {
+ GAIN_SCALE_ITIME_US(400000, BU27008_MEAS_MODE_400MS, 8),
+ GAIN_SCALE_ITIME_US(200000, BU27008_MEAS_MODE_200MS, 4),
+ GAIN_SCALE_ITIME_US(100000, BU27008_MEAS_MODE_100MS, 2),
+ GAIN_SCALE_ITIME_US(55000, BU27008_MEAS_MODE_55MS, 1),
+};
+
+/*
+ * All the RGBC channels share the same gain.
+ * IR gain can be fine-tuned from the gain set for the RGBC by 2 bit, but this
+ * would yield quite complex gain setting. Especially since not all bit
+ * compinations are supported. And in any case setting GAIN for RGBC will
+ * always also change the IR-gain.
+ *
+ * On top of this, the selector '0' which corresponds to hw-gain 1X on RGBC,
+ * corresponds to gain 2X on IR. Rest of the selctors correspond to same gains
+ * though. This, however, makes it not possible to use shared gain for all
+ * RGBC and IR settings even though they are all changed at the one go.
+ */
+#define BU27008_CHAN(color, data, separate_avail) \
+{ \
+ .type = IIO_INTENSITY, \
+ .modified = 1, \
+ .channel2 = IIO_MOD_LIGHT_##color, \
+ .info_mask_separate = BIT(IIO_CHAN_INFO_RAW) | \
+ BIT(IIO_CHAN_INFO_SCALE), \
+ .info_mask_separate_available = (separate_avail), \
+ .info_mask_shared_by_all = BIT(IIO_CHAN_INFO_INT_TIME), \
+ .info_mask_shared_by_all_available = BIT(IIO_CHAN_INFO_INT_TIME), \
+ .address = BU27008_REG_##data##_LO, \
+ .scan_index = BU27008_##color, \
+ .scan_type = { \
+ .sign = 's', \
+ .realbits = 16, \
+ .storagebits = 16, \
+ .endianness = IIO_LE, \
+ }, \
+}
+
+/* For raw reads we always configure DATA3 for CLEAR */
+static const struct iio_chan_spec bu27008_channels[] = {
+ BU27008_CHAN(RED, DATA0, BIT(IIO_CHAN_INFO_SCALE)),
+ BU27008_CHAN(GREEN, DATA1, BIT(IIO_CHAN_INFO_SCALE)),
+ BU27008_CHAN(BLUE, DATA2, BIT(IIO_CHAN_INFO_SCALE)),
+ BU27008_CHAN(CLEAR, DATA2, BIT(IIO_CHAN_INFO_SCALE)),
+ /*
+ * We don't allow setting scale for IR (because of shared gain bits).
+ * Hence we don't advertise available ones either.
+ */
+ BU27008_CHAN(IR, DATA3, 0),
+ IIO_CHAN_SOFT_TIMESTAMP(BU27008_NUM_CHANS),
+};
+
+struct bu27008_data {
+ struct regmap *regmap;
+ struct iio_trigger *trig;
+ struct device *dev;
+ struct iio_gts gts;
+ struct iio_gts gts_ir;
+ int64_t timestamp, old_timestamp;
+ int irq;
+
+ /*
+ * Prevent changing gain/time config when scale is read/written.
+ * Prevent changing gain/time when raw data is read.
+ */
+ struct mutex mutex;
+ bool trigger_enabled;
+
+ __le16 buffer[BU27008_NUM_CHANS];
+
+ struct {
+ __le16 channels[BU27008_NUM_CHANS];
+ s64 ts __aligned(8);
+ } scan;
+};
+
+static const struct regmap_range bu27008_volatile_ranges[] = {
+ {
+ .range_min = BU27008_REG_SYSTEM_CONTROL, /* SWRESET */
+ .range_max = BU27008_REG_SYSTEM_CONTROL,
+ }, {
+ .range_min = BU27008_REG_MODE_CONTROL3, /* VALID */
+ .range_max = BU27008_REG_MODE_CONTROL3,
+ }, {
+ .range_min = BU27008_REG_DATA0_LO, /* DATA */
+ .range_max = BU27008_REG_DATA3_HI,
+ },
+};
+
+static const struct regmap_access_table bu27008_volatile_regs = {
+ .yes_ranges = &bu27008_volatile_ranges[0],
+ .n_yes_ranges = ARRAY_SIZE(bu27008_volatile_ranges),
+};
+
+static const struct regmap_range bu27008_read_only_ranges[] = {
+ {
+ .range_min = BU27008_REG_DATA0_LO,
+ .range_max = BU27008_REG_DATA3_HI,
+ }, {
+ .range_min = BU27008_REG_MANUFACTURER_ID,
+ .range_max = BU27008_REG_MANUFACTURER_ID,
+ }
+};
+
+static const struct regmap_access_table bu27008_ro_regs = {
+ .no_ranges = &bu27008_read_only_ranges[0],
+ .n_no_ranges = ARRAY_SIZE(bu27008_read_only_ranges),
+};
+
+static const struct regmap_config bu27008_regmap = {
+ .reg_bits = 8,
+ .val_bits = 8,
+ .max_register = BU27008_REG_MAX,
+ .cache_type = REGCACHE_RBTREE,
+ .volatile_table = &bu27008_volatile_regs,
+ .wr_table = &bu27008_ro_regs,
+};
+
+#define BU27008_MAX_VALID_RESULT_WAIT_US 50000
+#define BU27008_VALID_RESULT_WAIT_QUANTA_US 1000
+
+static int bu27008_chan_read_data(struct bu27008_data *data, int reg, int *val)
+{
+ int ret, valid;
+ __le16 tmp;
+
+ ret = regmap_read_poll_timeout(data->regmap, BU27008_REG_MODE_CONTROL3,
+ valid, (valid & BU27008_MASK_VALID),
+ BU27008_VALID_RESULT_WAIT_QUANTA_US,
+ BU27008_MAX_VALID_RESULT_WAIT_US);
+ if (ret)
+ return ret;
+
+ ret = regmap_bulk_read(data->regmap, reg, &tmp, sizeof(tmp));
+ if (ret)
+ dev_err(data->dev, "Reading channel data failed\n");
+
+ *val = le16_to_cpu(tmp);
+
+ return ret;
+}
+
+static int bu27008_get_gain(struct bu27008_data *data, struct iio_gts *gts, int *gain)
+{
+ int ret, sel;
+
+ ret = regmap_read(data->regmap, BU27008_REG_MODE_CONTROL2, &sel);
+ if (ret)
+ return ret;
+
+ sel = FIELD_GET(BU27008_MASK_RGBC_GAIN, sel);
+
+ ret = iio_gts_find_gain_by_sel(gts, sel);
+
+ if (ret < 0) {
+ dev_err(data->dev, "unknown gain value 0x%x\n", sel);
+
+ return ret;
+ }
+
+ *gain = ret;
+
+ return 0;
+}
+
+static int bu27008_write_gain_sel(struct bu27008_data *data, int sel)
+{
+ int regval;
+
+ regval = FIELD_PREP(BU27008_MASK_RGBC_GAIN, sel);
+
+ /*
+ * We do always set also the LOW bits of IR-gain because othervice we
+ * would risk resulting an invalid GAIN register value.
+ *
+ * We could allow setting separate gains for RGBC and IR when the
+ * values were such that HW could support both gain settings.
+ * Eg, when the shared bits were same for both gain values.
+ *
+ * This, however, has a negligible benefit compared to the increased
+ * software complexity when we would need to go through the gains
+ * for both channels separately when the integration time changes.
+ * This would end up with nasty logic for computing gain values for
+ * both channels - and rejecting them if shared bits changed.
+ *
+ * We should then build the logic by guessing what a user prefers.
+ * RGBC or IR gains correctly set while other jumps to odd value?
+ * Maybe look-up a value where both gains are somehow optimized
+ * <what this somehow is, is ATM unknown to us>. Or maybe user would
+ * expect us to reject changes when optimal gains can't be set to both
+ * channels w/given integration time. At best that would result
+ * solution that works well for a very specific subset of
+ * configurations but causes unexpected corner-cases.
+ *
+ * So, we keep it simple. Always set same selector to IR and RGBC.
+ * We disallow setting IR (as I expect that most of the users are
+ * interested in RGBC). This way we can show the user that the scales
+ * for RGBC and IR channels are different (1X Vs 2X with sel 0) while
+ * still keeping the operation deterministic.
+ */
+ regval |= FIELD_PREP(BU27008_MASK_IR_GAIN_LO, sel);
+
+ return regmap_update_bits(data->regmap, BU27008_REG_MODE_CONTROL2,
+ BU27008_MASK_RGBC_GAIN, regval);
+}
+
+static int bu27008_set_gain(struct bu27008_data *data, int gain)
+{
+ int ret;
+
+ ret = iio_gts_find_sel_by_gain(&data->gts, gain);
+ if (ret < 0)
+ return ret;
+
+ return bu27008_write_gain_sel(data, ret);
+}
+
+static int bu27008_get_int_time_sel(struct bu27008_data *data, int *sel)
+{
+ int ret, val;
+
+ ret = regmap_read(data->regmap, BU27008_REG_MODE_CONTROL1, &val);
+ *sel = FIELD_GET(BU27008_MASK_MEAS_MODE, val);
+
+ return ret;
+}
+
+static int bu27008_set_int_time_sel(struct bu27008_data *data, int sel)
+{
+ return regmap_update_bits(data->regmap, BU27008_REG_MODE_CONTROL1,
+ BU27008_MASK_MEAS_MODE, sel);
+}
+
+static int bu27008_get_int_time(struct bu27008_data *data)
+{
+ int ret, sel;
+
+ ret = bu27008_get_int_time_sel(data, &sel);
+ if (ret)
+ return ret;
+
+ return iio_gts_find_int_time_by_sel(&data->gts,
+ sel & BU27008_MASK_MEAS_MODE);
+}
+
+static int _bu27008_get_scale(struct bu27008_data *data, bool ir, int *val,
+ int *val2)
+{
+ struct iio_gts *gts;
+ int gain, ret;
+
+ if (ir)
+ gts = &data->gts_ir;
+ else
+ gts = &data->gts;
+
+ ret = bu27008_get_gain(data, gts, &gain);
+ if (ret)
+ return ret;
+
+ ret = bu27008_get_int_time(data);
+ if (ret < 0)
+ return ret;
+
+ return iio_gts_get_scale(gts, gain, ret, val, val2);
+}
+
+static int bu27008_get_scale(struct bu27008_data *data, bool ir, int *val,
+ int *val2)
+{
+ int ret;
+
+ mutex_lock(&data->mutex);
+ ret = _bu27008_get_scale(data, ir, val, val2);
+ mutex_unlock(&data->mutex);
+
+ return ret;
+}
+
+static int bu27008_set_int_time(struct bu27008_data *data, int time)
+{
+ int ret;
+
+ ret = iio_gts_find_sel_by_int_time(&data->gts, time);
+ if (ret < 0)
+ return ret;
+
+ return regmap_update_bits(data->regmap, BU27008_REG_MODE_CONTROL1,
+ BU27008_MASK_MEAS_MODE, ret);
+}
+
+/* Try to change the time so that the scale is maintained */
+static int bu27008_try_set_int_time(struct bu27008_data *data, int int_time_new)
+{
+ int ret, old_time_sel, new_time_sel, old_gain, new_gain;
+
+ mutex_lock(&data->mutex);
+
+ ret = bu27008_get_int_time_sel(data, &old_time_sel);
+ if (ret < 0)
+ goto unlock_out;
+
+ if (!iio_gts_valid_time(&data->gts, int_time_new)) {
+ dev_dbg(data->dev, "Unsupported integration time %u\n",
+ int_time_new);
+
+ ret = -EINVAL;
+ goto unlock_out;
+ }
+ new_time_sel = iio_gts_find_sel_by_int_time(&data->gts, int_time_new);
+ if (new_time_sel == old_time_sel) {
+ ret = 0;
+ goto unlock_out;
+ }
+
+ ret = bu27008_get_gain(data, &data->gts, &old_gain);
+ if (ret)
+ goto unlock_out;
+
+ ret = iio_gts_find_new_gain_sel_by_old_gain_time(&data->gts, old_gain,
+ old_time_sel, new_time_sel, &new_gain);
+ if (ret) {
+ int scale1, scale2;
+ bool ok;
+
+ _bu27008_get_scale(data, false, &scale1, &scale2);
+ dev_dbg(data->dev,
+ "Can't support time %u with current scale %u %u\n",
+ int_time_new, scale1, scale2);
+
+ if (new_gain < 0)
+ goto unlock_out;
+
+ /*
+ * If caller requests for integration time change and we
+ * can't support the scale - then the caller should be
+ * prepared to 'pick up the pieces and deal with the
+ * fact that the scale changed'.
+ */
+ ret = iio_find_closest_gain_low(&data->gts, new_gain, &ok);
+ if (!ok)
+ dev_dbg(data->dev, "optimal gain out of range\n");
+
+ if (ret < 0) {
+ dev_dbg(data->dev,
+ "Total gain increase. Risk of saturation");
+ ret = iio_gts_get_min_gain(&data->gts);
+ if (ret < 0)
+ goto unlock_out;
+ }
+ new_gain = ret;
+ dev_dbg(data->dev, "scale changed, new gain %u\n", new_gain);
+ }
+
+ ret = bu27008_set_gain(data, new_gain);
+ if (ret)
+ goto unlock_out;
+
+ ret = bu27008_set_int_time(data, int_time_new);
+
+unlock_out:
+ mutex_unlock(&data->mutex);
+
+ return ret;
+}
+
+static int bu27008_meas_set(struct bu27008_data *data, bool enable)
+{
+ if (enable)
+ return regmap_set_bits(data->regmap, BU27008_REG_MODE_CONTROL3,
+ BU27008_MASK_MEAS_EN);
+
+ return regmap_clear_bits(data->regmap, BU27008_REG_MODE_CONTROL3,
+ BU27008_MASK_MEAS_EN);
+}
+
+static int bu27008_chan_cfg(struct bu27008_data *data,
+ struct iio_chan_spec const *chan)
+{
+ int chan_sel;
+
+ if (chan->scan_index == BU27008_BLUE)
+ chan_sel = BU27008_BLUE2_CLEAR3;
+ else
+ chan_sel = BU27008_CLEAR2_IR3;
+
+ chan_sel = FIELD_PREP(BU27008_MASK_CHAN_SEL, chan_sel);
+
+ return regmap_update_bits(data->regmap, BU27008_REG_MODE_CONTROL3,
+ BU27008_MASK_CHAN_SEL, chan_sel);
+}
+
+static int bu27008_read_one(struct bu27008_data *data, struct iio_dev *idev,
+ struct iio_chan_spec const *chan, int *val, int *val2)
+{
+ int ret, int_time;
+
+ ret = bu27008_chan_cfg(data, chan);
+ if (ret)
+ return ret;
+
+ ret = bu27008_meas_set(data, true);
+ if (ret)
+ return ret;
+
+ int_time = bu27008_get_int_time(data);
+ if (int_time < 0)
+ int_time = 400000;
+
+ msleep((int_time + 500) / 1000);
+
+ ret = bu27008_chan_read_data(data, chan->address, val);
+ if (!ret)
+ ret = IIO_VAL_INT;
+
+ if (bu27008_meas_set(data, false))
+ dev_warn(data->dev, "measurement disabling failed\n");
+
+ return ret;
+}
+
+static int bu27008_read_raw(struct iio_dev *idev,
+ struct iio_chan_spec const *chan,
+ int *val, int *val2, long mask)
+{
+ struct bu27008_data *data = iio_priv(idev);
+ int busy, ret;
+
+ switch (mask) {
+ case IIO_CHAN_INFO_RAW:
+ {
+ busy = iio_device_claim_direct_mode(idev);
+ if (busy)
+ return -EBUSY;
+
+ mutex_lock(&data->mutex);
+ ret = bu27008_read_one(data, idev, chan, val, val2);
+ mutex_unlock(&data->mutex);
+
+ iio_device_release_direct_mode(idev);
+
+ return ret;
+ }
+ case IIO_CHAN_INFO_SCALE:
+ ret = bu27008_get_scale(data, chan->scan_index == BU27008_IR,
+ val, val2);
+ if (ret)
+ return ret;
+
+ return IIO_VAL_INT_PLUS_NANO;
+
+ case IIO_CHAN_INFO_INT_TIME:
+ ret = bu27008_get_int_time(data);
+ if (ret < 0)
+ return ret;
+
+ *val = 0;
+ *val2 = ret;
+
+ return IIO_VAL_INT_PLUS_MICRO;
+
+ default:
+ return -EINVAL;
+ }
+}
+
+static int bu27008_set_scale(struct bu27008_data *data,
+ struct iio_chan_spec const *chan,
+ int val, int val2)
+{
+ int ret, gain_sel, time_sel, i;
+
+ if (chan->scan_index == BU27008_IR)
+ return -EINVAL;
+
+ mutex_lock(&data->mutex);
+
+ ret = bu27008_get_int_time_sel(data, &time_sel);
+ if (ret < 0)
+ goto unlock_out;
+
+
+ ret = iio_gts_find_gain_sel_for_scale_using_time(&data->gts, time_sel,
+ val, val2 * 1000, &gain_sel);
+ if (ret) {
+ /* Could not support new scale with existing int-time */
+ int new_time_sel;
+
+ for (i = 0; i < data->gts.num_itime; i++) {
+ new_time_sel = data->gts.itime_table[i].sel;
+ ret = iio_gts_find_gain_sel_for_scale_using_time(
+ &data->gts, new_time_sel, val, val2 * 1000,
+ &gain_sel);
+ if (!ret)
+ break;
+ }
+ if (i == data->gts.num_itime) {
+ dev_err(data->dev, "Can't support scale %u %u\n", val,
+ val2);
+
+ ret = -EINVAL;
+ goto unlock_out;
+ }
+
+ ret = bu27008_set_int_time_sel(data, new_time_sel);
+ if (ret)
+ goto unlock_out;
+ }
+
+ ret = bu27008_write_gain_sel(data, gain_sel);
+
+unlock_out:
+ mutex_unlock(&data->mutex);
+
+ return ret;
+}
+
+static int bu27008_write_raw(struct iio_dev *idev,
+ struct iio_chan_spec const *chan,
+ int val, int val2, long mask)
+{
+ struct bu27008_data *data = iio_priv(idev);
+ int ret;
+
+ /*
+ * We should not allow changing scale when measurement is ongoing.
+ * This could make values in buffer inconsistent.
+ */
+ ret = iio_device_claim_direct_mode(idev);
+ if (ret)
+ return ret;
+
+ switch (mask) {
+ case IIO_CHAN_INFO_SCALE:
+ ret = bu27008_set_scale(data, chan, val, val2);
+ break;
+ case IIO_CHAN_INFO_INT_TIME:
+ if (val)
+ ret = -EINVAL;
+ else
+ ret = bu27008_try_set_int_time(data, val2);
+ break;
+ default:
+ ret = -EINVAL;
+ break;
+ }
+ iio_device_release_direct_mode(idev);
+
+ return ret;
+}
+
+static int bu27008_validate_trigger(struct iio_dev *idev,
+ struct iio_trigger *trig)
+{
+ struct bu27008_data *data = iio_priv(idev);
+
+ if (data->trig != trig)
+ return -EINVAL;
+
+ return 0;
+}
+
+static int bu27008_read_avail(struct iio_dev *idev,
+ struct iio_chan_spec const *chan, const int **vals,
+ int *type, int *length, long mask)
+{
+ struct bu27008_data *data = iio_priv(idev);
+
+ switch (mask) {
+ case IIO_CHAN_INFO_INT_TIME:
+ return iio_gts_avail_times(&data->gts, vals, type, length);
+ case IIO_CHAN_INFO_SCALE:
+ if (chan->channel2 == IIO_MOD_LIGHT_IR)
+ return iio_gts_all_avail_scales(&data->gts_ir, vals,
+ type, length);
+ return iio_gts_all_avail_scales(&data->gts, vals, type, length);
+ default:
+ return -EINVAL;
+ }
+}
+
+static const struct iio_info bu27008_info = {
+ .read_raw = &bu27008_read_raw,
+ .write_raw = &bu27008_write_raw,
+ .read_avail = &bu27008_read_avail,
+ .validate_trigger = bu27008_validate_trigger,
+};
+
+static int bu27008_chip_init(struct bu27008_data *data)
+{
+ int ret;
+
+ /* Reset */
+ ret = regmap_update_bits(data->regmap, BU27008_REG_SYSTEM_CONTROL,
+ BU27008_MASK_SW_RESET, BU27008_MASK_SW_RESET);
+ if (ret)
+ return dev_err_probe(data->dev, ret, "Sensor reset failed\n");
+
+ msleep(1);
+
+ return ret;
+}
+
+static int bu27008_set_drdy_irq(struct bu27008_data *data, bool state)
+{
+ if (state)
+ return regmap_set_bits(data->regmap, BU27008_REG_MODE_CONTROL3,
+ BU27008_MASK_INT_EN);
+ return regmap_clear_bits(data->regmap, BU27008_REG_MODE_CONTROL3,
+ BU27008_MASK_INT_EN);
+}
+
+static int bu27008_trigger_set_state(struct iio_trigger *trig,
+ bool state)
+{
+ struct bu27008_data *data = iio_trigger_get_drvdata(trig);
+ int ret = 0;
+
+ mutex_lock(&data->mutex);
+
+ if (data->trigger_enabled != state) {
+ data->trigger_enabled = state;
+ ret = bu27008_set_drdy_irq(data, state);
+ if (ret)
+ dev_err(data->dev, "Failed to set trigger state\n");
+ }
+ mutex_unlock(&data->mutex);
+
+ return ret;
+}
+
+static const struct iio_trigger_ops bu27008_trigger_ops = {
+ .set_trigger_state = bu27008_trigger_set_state,
+};
+
+static irqreturn_t bu27008_irq_handler(int irq, void *private)
+{
+ struct iio_dev *idev = private;
+ struct bu27008_data *data = iio_priv(idev);
+
+ data->old_timestamp = data->timestamp;
+ data->timestamp = iio_get_time_ns(idev);
+
+ if (data->trigger_enabled)
+ return IRQ_WAKE_THREAD;
+
+ return IRQ_NONE;
+}
+
+static irqreturn_t bu27008_trigger_handler(int irq, void *p)
+{
+ struct iio_poll_func *pf = p;
+ struct iio_dev *idev = pf->indio_dev;
+ struct bu27008_data *data = iio_priv(idev);
+ __le16 raw[BU27008_NUM_CHANS + NUM_U16_IN_TSTAMP];
+ int ret, dummy;
+
+ memset(&raw, 0, sizeof(raw));
+
+ /*
+ * After some measurements, it seems reading the
+ * BU27008_REG_MODE_CONTROL3 debounces the IRQ line
+ */
+ ret = regmap_read(data->regmap, BU27008_REG_MODE_CONTROL3, &dummy);
+ if (ret < 0)
+ goto err_read;
+
+ ret = regmap_bulk_read(data->regmap, BU27008_REG_DATA0_LO, data->buffer,
+ BU27008_HW_DATA_SIZE);
+ if (ret < 0)
+ goto err_read;
+
+ /* Red and green are always in dedicated channels. */
+ if (*idev->active_scan_mask & BIT(BU27008_RED))
+ raw[BU27008_RED] = data->buffer[BU27008_RED];
+ if (*idev->active_scan_mask & BIT(BU27008_GREEN))
+ raw[BU27008_GREEN] = data->buffer[BU27008_GREEN];
+
+ /*
+ * We need to check the scan mask to determine which of the
+ * BLUE/CLEAR/IR are enabled so we know which channel is used to
+ * measure which data.
+ */
+ if (*idev->active_scan_mask & BIT(BU27008_BLUE)) {
+ raw[BU27008_BLUE] = data->buffer[BU27008_DATA2];
+
+ if (*idev->active_scan_mask & BIT(BU27008_CLEAR))
+ raw[BU27008_CLEAR] = data->buffer[BU27008_DATA3];
+ } else {
+ if (*idev->active_scan_mask & BIT(BU27008_CLEAR))
+ raw[BU27008_CLEAR] = data->buffer[BU27008_DATA2];
+ }
+ if (*idev->active_scan_mask & BIT(BU27008_IR))
+ raw[BU27008_IR] = data->buffer[BU27008_DATA3];
+
+ iio_push_to_buffers_with_timestamp(idev, raw, pf->timestamp);
+err_read:
+ iio_trigger_notify_done(idev->trig);
+
+ return IRQ_HANDLED;
+}
+
+static irqreturn_t bu27008_irq_thread_handler(int irq, void *private)
+{
+ struct iio_dev *idev = private;
+ struct bu27008_data *data = iio_priv(idev);
+ irqreturn_t ret = IRQ_NONE;
+
+ mutex_lock(&data->mutex);
+ if (data->trigger_enabled) {
+ iio_trigger_poll_nested(data->trig);
+ ret = IRQ_HANDLED;
+ }
+ mutex_unlock(&data->mutex);
+
+ return ret;
+}
+
+static int bu27008_buffer_preenable(struct iio_dev *idev)
+{
+ struct bu27008_data *data = iio_priv(idev);
+ int chan_sel, ret;
+
+ /* Configure channel selection */
+ if (*idev->active_scan_mask & BIT(BU27008_BLUE)) {
+ if (*idev->active_scan_mask & BIT(BU27008_CLEAR))
+ chan_sel = BU27008_BLUE2_CLEAR3;
+ else
+ chan_sel = BU27008_BLUE2_IR3;
+ } else {
+ chan_sel = BU27008_CLEAR2_IR3;
+ }
+
+ chan_sel = FIELD_PREP(BU27008_MASK_CHAN_SEL, chan_sel);
+
+ ret = regmap_update_bits(data->regmap, BU27008_REG_MODE_CONTROL3,
+ BU27008_MASK_CHAN_SEL, chan_sel);
+ if (ret)
+ return ret;
+
+ return bu27008_meas_set(data, true);
+}
+
+static int bu27008_buffer_postdisable(struct iio_dev *idev)
+{
+ struct bu27008_data *data = iio_priv(idev);
+
+ return bu27008_meas_set(data, false);
+}
+
+static const struct iio_buffer_setup_ops bu27008_buffer_ops = {
+ .preenable = bu27008_buffer_preenable,
+ .postdisable = bu27008_buffer_postdisable,
+};
+
+static int bu27008_probe(struct i2c_client *i2c)
+{
+ struct device *dev = &i2c->dev;
+ struct iio_trigger *indio_trig;
+ struct bu27008_data *data;
+ struct regmap *regmap;
+ unsigned int part_id, reg;
+ struct iio_dev *idev;
+ char *name;
+ int ret;
+
+ if (!i2c->irq) {
+ dev_err(dev, "No IRQ configured\n");
+ return -EINVAL;
+ }
+
+ regmap = devm_regmap_init_i2c(i2c, &bu27008_regmap);
+ if (IS_ERR(regmap))
+ return dev_err_probe(dev, PTR_ERR(regmap),
+ "Failed to initialize Regmap\n");
+
+ idev = devm_iio_device_alloc(dev, sizeof(*data));
+ if (!idev)
+ return -ENOMEM;
+
+ ret = devm_regulator_get_enable(dev, "vdd");
+ if (ret)
+ return dev_err_probe(dev, ret, "Failed to get regulator\n");
+
+ data = iio_priv(idev);
+
+ ret = regmap_read(regmap, BU27008_REG_SYSTEM_CONTROL, ®);
+ if (ret)
+ return dev_err_probe(dev, ret, "Failed to access sensor\n");
+
+ part_id = FIELD_GET(BU27008_MASK_PART_ID, reg);
+
+ if (part_id != BU27008_ID)
+ dev_warn(dev, "unknown device 0x%x\n", part_id);
+
+ ret = devm_iio_init_iio_gts(dev, BU27008_SCALE_1X, 0, bu27008_gains,
+ ARRAY_SIZE(bu27008_gains), bu27008_itimes,
+ ARRAY_SIZE(bu27008_itimes), &data->gts);
+ if (ret)
+ return ret;
+
+ ret = devm_iio_init_iio_gts(dev, BU27008_SCALE_1X, 0, bu27008_gains_ir,
+ ARRAY_SIZE(bu27008_gains_ir), bu27008_itimes,
+ ARRAY_SIZE(bu27008_itimes), &data->gts_ir);
+ if (ret)
+ return ret;
+
+ mutex_init(&data->mutex);
+ data->regmap = regmap;
+ data->dev = dev;
+ data->irq = i2c->irq;
+
+ idev->channels = bu27008_channels;
+ idev->num_channels = ARRAY_SIZE(bu27008_channels);
+ idev->name = "bu27008";
+ idev->info = &bu27008_info;
+ idev->modes = INDIO_DIRECT_MODE | INDIO_BUFFER_SOFTWARE;
+ idev->available_scan_masks = bu27008_scan_masks;
+
+ ret = bu27008_chip_init(data);
+ if (ret)
+ return ret;
+ ret = devm_iio_triggered_buffer_setup_ext(dev, idev,
+ &iio_pollfunc_store_time,
+ bu27008_trigger_handler,
+ IIO_BUFFER_DIRECTION_IN,
+ &bu27008_buffer_ops,
+ NULL);
+ if (ret)
+ return dev_err_probe(data->dev, ret,
+ "iio_triggered_buffer_setup_ext FAIL\n");
+
+ indio_trig = devm_iio_trigger_alloc(dev, "%sdata-rdy-dev%d", idev->name,
+ iio_device_id(idev));
+ if (!indio_trig)
+ return -ENOMEM;
+
+ data->trig = indio_trig;
+
+ indio_trig->ops = &bu27008_trigger_ops;
+ iio_trigger_set_drvdata(indio_trig, data);
+
+ /*
+ * No need to check for NULL. request_threaded_irq() defaults to
+ * dev_name() should the alloc fail.
+ */
+ name = devm_kasprintf(data->dev, GFP_KERNEL, "%s-bu27008",
+ dev_name(data->dev));
+
+ ret = devm_request_threaded_irq(data->dev, i2c->irq, bu27008_irq_handler,
+ &bu27008_irq_thread_handler,
+ IRQF_ONESHOT, name, idev);
+ if (ret)
+ return dev_err_probe(data->dev, ret, "Could not request IRQ\n");
+
+
+ ret = devm_iio_trigger_register(dev, indio_trig);
+ if (ret)
+ return dev_err_probe(data->dev, ret,
+ "Trigger registration failed\n");
+
+ ret = devm_iio_device_register(data->dev, idev);
+ if (ret < 0)
+ return dev_err_probe(dev, ret,
+ "Unable to register iio device\n");
+
+ return ret;
+}
+
+static const struct of_device_id bu27008_of_match[] = {
+ { .compatible = "rohm,bu27008", },
+ { }
+};
+MODULE_DEVICE_TABLE(of, bu27008_of_match);
+
+static struct i2c_driver bu27008_i2c_driver = {
+ .driver = {
+ .name = "bu27008",
+ .of_match_table = bu27008_of_match,
+ },
+ .probe_new = bu27008_probe,
+};
+module_i2c_driver(bu27008_i2c_driver);
+
+MODULE_DESCRIPTION("ROHM BU27008 colour sensor driver");
+MODULE_AUTHOR("Matti Vaittinen <[email protected]>");
+MODULE_LICENSE("GPL");
+MODULE_IMPORT_NS(IIO_GTS_HELPER);
--
2.40.0
--
Matti Vaittinen, Linux device drivers
ROHM Semiconductors, Finland SWDC
Kiviharjunlenkki 1E
90220 OULU
FINLAND
~~~ "I don't think so," said Rene Descartes. Just then he vanished ~~~
Simon says - in Latin please.
~~~ "non cogito me" dixit Rene Descarte, deinde evanescavit ~~~
Thanks to Simon Glass for the translation =]
On Fri, 21 Apr 2023 12:37:30 +0300
Matti Vaittinen <[email protected]> wrote:
> Add support for ROHM BU27008 RGB sensor.
>
> The ROHM BU27008 is a sensor with 5 photodiodes (red, green, blue, clear
> and IR) with four configurable channels. Red and green being always
> available and two out of the rest three (blue, clear, IR) can be
> selected to be simultaneously measured. Typical application is adjusting
> LCD backlight of TVs, mobile phones and tablet PCs.
>
> This series supports reading the RGBC and IR channels using IIO
> frameeork. However, only two of the BC+IR can be enabled at the same
framework
> time. Series adds also support for scale and integration time
> configuration, where scale consists of impact of both the integration
> time and hardware gain. The gain and time support is backed by the newly
> introduced IIO GTS helper. This series depends on GTS helper patches
> added in BU27034 support series which is already merged in iio/togreg
> which this series is based on.
>
> The hardware allows configuring gain setting by writing a 5-bit gain
> selector value to a register. Part of the gain setting is common for all
> channels (RGBC + IR) but part of the selector value can be set
> separately for RGBC and IR:
>
> MODE_CONTROL2 REG:
> bit 7 6 5 4 3 2 1 0
> -----------------------------------------------------------------
> | RGB selector |
> +---------------------------------------+
> -----------------------------------------------------------------
> | high bits IR | | low bits IR selector |
> +---------------+ +-----------------------+
>
> In theory it would be possible to set certain separate gain values for
> RGBC and IR channels, but this gets pretty confusing because there are a
> few 'unsupported' selector values. If only RGBC or IR was set, some
> extra handling should be done to prevent the other channel from getting
> unsupported value due to change in high-bits. Furthermore, allowing the
> channels to be set different gain values (in some cases when gains are
> such the HW supports it) would make the cases where also integration
> time is changed to achieve correct scale ... interesting. It might also
> be confusing for user to try predicting when setting different scales
> succeeds and when it does not. Furthermore, if for example the scale
> setting for RGBC caused IR selector to be invalid - it could also cause
> the IR scale to "jump" very far from previous value.
>
> To make the code simpler and more predictable for users, the current
> logic is as follows:
>
> 1. Prevent setting IR scale. (My assumption is IR is less used than
> RGBC)
> 2. When RGBC scale is set, set also the IR-selector to the same value.
> This prevents unsupported selector values and makes the IR scale changes
> predictable.
>
> The 2) could mean we effectively have the same scale for all channels.
> Unfortunately, the HW design is slightly peculiar and selector 0 means
> gain 1X on RGBC but gain 2X on IR. Rest of the selectors equal same gain
> values on RGBC and IR. The result is that while changin selector from 0
> => 1 causes RGBC gain to go from 1X => 4X, it causes IR gain to go from
> 2X => 4X.
>
> So, the driver provides separate scale entries for all channels (also
> RGB and C will have separate gain entries because these channels are of
> same type as IR channel). This makes it possible for user applications
> to go read the scales for all channels after setting scale for one (in
> order to detect the IR scale difference).
>
> Having the separate IR scale entry which applications can read to detect
> "arbitrary scale changes" makes it possible for applications to be
> written so they can cope if we need to implement the 'allow setting some
> different gains for IR and RGBC' - later.
Hmm. I'm not sure preventing it is the best approach. That makes for an
interface that is perhaps even less intuitive than having it affect the
scales of the other channels. Still having it configurable at all is
optional from an ABI point of view, so we could go with what you have
here and see if anyone shouts about it in future.
Jonathan
>
> Finally, the scales_available is also provided for all other channels
> except the IR channel, which does not allow the scale to be changed.
>
> The sensor provides a data-ready IRQ and the driver implements a
> triggered buffer mode using this IRQ as a trigger.
>
> ---
>
> Matti Vaittinen (3):
> dt-bindings: iio: light: ROHM BU27008
> iio: light: ROHM BU27008 color sensor
> MAINTAINERS: Add ROHM BU27008
>
> .../bindings/iio/light/rohm-bu27008.yaml | 49 +
> MAINTAINERS | 3 +-
> drivers/iio/light/Kconfig | 14 +
> drivers/iio/light/Makefile | 1 +
> drivers/iio/light/rohm-bu27008.c | 1028 +++++++++++++++++
> 5 files changed, 1094 insertions(+), 1 deletion(-)
> create mode 100644 Documentation/devicetree/bindings/iio/light/rohm-bu27008.yaml
> create mode 100644 drivers/iio/light/rohm-bu27008.c
>
>
> base-commit: 52cc189b4fc6af6accc45fe7b7053d76d8724059
On Fri, 21 Apr 2023 12:39:36 +0300
Matti Vaittinen <[email protected]> wrote:
> The ROHM BU27008 is a sensor with 5 photodiodes (red, green, blue, clear
> and IR) with four configurable channels. Red and green being always
> available and two out of the rest three (blue, clear, IR) can be
> selected to be simultaneously measured. Typical application is adjusting
> LCD backlight of TVs, mobile phones and tablet PCs.
>
> Add initial support for the ROHM BU27008 color sensor.
> - raw_read() of RGB and clear channels
> - triggered buffer w/ DRDY interrtupt
>
> Signed-off-by: Matti Vaittinen <[email protected]>
Hi Matti,
Biggest issue in here is some confusion over data packing when some channels
are enabled. The driver must pack the channels that are enabled (seen
from active_scan_mask) according to general IIO rules. So naturally aligned
buffers. Thus for this device it should always be packed into
struct scan {
__le16 chans[4];
s64 ts __aligned(8); /*it's aligned anyway but better not to make reviewers think ;) */
};
Even though there are 5 possible channels. If one in the middle isnt' enabled (e.g. blue)
then clear and IR shift to lower words. of the buffer.
The demux code in the IIO core then deals with userspace requesting less than this
by repacking the data if needed. That allows it to present different views to different
consumers (e.g. userspace IIO access, and an inkernel buffer user - though there aren't
any of those for light sensors AFAIK.
I'd not thought about the issue of the weird scales interacting with someone changing
the integration time. That's nasty and I don't have a better suggestion than what you have
currently.
Otherwise some trivial stuff inline.
Jonathan
> diff --git a/drivers/iio/light/rohm-bu27008.c b/drivers/iio/light/rohm-bu27008.c
> new file mode 100644
> index 000000000000..6fca193eeb9e
> --- /dev/null
> +++ b/drivers/iio/light/rohm-bu27008.c
> @@ -0,0 +1,1028 @@
...
> +
> +enum {
> + BU27008_RED, /* Always data0 */
> + BU27008_GREEN, /* Always data1 */
> + BU27008_BLUE, /* data2, configurable (blue / clear) */
> + BU27008_CLEAR, /* data2 or data3 */
> + BU27008_IR, /* data3 */
> + BU27008_NUM_CHANS
> +};
> +
> +enum {
> + BU27008_DATA0, /* Always RED */
> + BU27008_DATA1, /* Always GREEN */
> + BU27008_DATA2, /* Blue or Clear */
> + BU27008_DATA3, /* IR or Clear */
> + BU27008_NUM_HW_CHANS
> +};
> +
> +/* We can always measure red and green at same time */
> +#define ALWAYS_SCANNABLE (BIT(BU27008_RED) | BIT(BU27008_GREEN))
> +
> +#define BU27008_CHAN_DATA_SIZE 2 /* Each channel has 16bits of data */
> +#define BU27008_BUF_DATA_SIZE (BU27008_NUM_CHANS * BU27008_CHAN_DATA_SIZE)
Buffer should be same size as the number you can grab in one read. So
it should be same as BU27008_HW_DATA_SIZE. The data is packed when only some
channels are enabled.
> +#define BU27008_HW_DATA_SIZE (BU27008_NUM_HW_CHANS * BU27008_CHAN_DATA_SIZE)
> +#define NUM_U16_IN_TSTAMP (sizeof(s64) / sizeof(u16))
> +
> +static const unsigned long bu27008_scan_masks[] = {
> + ALWAYS_SCANNABLE | BIT(BU27008_CLEAR) | BIT(BU27008_IR),
> + ALWAYS_SCANNABLE | BIT(BU27008_CLEAR) | BIT(BU27008_BLUE),
> + ALWAYS_SCANNABLE | BIT(BU27008_BLUE) | BIT(BU27008_IR),
Good, this looks correct for allowing the IIO core to deal with demuxing
the data as necessary. Note that this is saying that if one of these
modes is active scan mask it will be backed into lowers set of __le16
buffer elements.
> + 0
> +};
> +
> +
> +struct bu27008_data {
> + struct regmap *regmap;
> + struct iio_trigger *trig;
> + struct device *dev;
> + struct iio_gts gts;
> + struct iio_gts gts_ir;
> + int64_t timestamp, old_timestamp;
As noted later, not sure what old_timestamp is for.
> + int irq;
> +
> + /*
> + * Prevent changing gain/time config when scale is read/written.
> + * Prevent changing gain/time when raw data is read.
Does more than that. Probably need a bit more detail here.
> + */
> + struct mutex mutex;
> + bool trigger_enabled;
> +
> + __le16 buffer[BU27008_NUM_CHANS];
As below, you should be able to always read directly into the element you
also use to push to the iio buffer.
> +
> + struct {
> + __le16 channels[BU27008_NUM_CHANS];
> + s64 ts __aligned(8);
> + } scan;
As noted below. This has write structure for a buffer scan, but you don't use it.
Mind you, no need to have it in here that I can see. I'd just put it on the stack.
> +};
> +
> +static int bu27008_get_gain(struct bu27008_data *data, struct iio_gts *gts, int *gain)
> +{
> + int ret, sel;
> +
> + ret = regmap_read(data->regmap, BU27008_REG_MODE_CONTROL2, &sel);
> + if (ret)
> + return ret;
> +
> + sel = FIELD_GET(BU27008_MASK_RGBC_GAIN, sel);
> +
> + ret = iio_gts_find_gain_by_sel(gts, sel);
> +
No blank line here. Keep call and it's error handler in one block of code.
> + if (ret < 0) {
> + dev_err(data->dev, "unknown gain value 0x%x\n", sel);
> +
Blank line here doesn't add anything. Drop it.
> + return ret;
> + }
> +
> + *gain = ret;
> +
> + return 0;
> +}
> +
> +static int bu27008_write_gain_sel(struct bu27008_data *data, int sel)
> +{
> + int regval;
> +
> + regval = FIELD_PREP(BU27008_MASK_RGBC_GAIN, sel);
> +
> + /*
> + * We do always set also the LOW bits of IR-gain because othervice we
> + * would risk resulting an invalid GAIN register value.
> + *
> + * We could allow setting separate gains for RGBC and IR when the
> + * values were such that HW could support both gain settings.
> + * Eg, when the shared bits were same for both gain values.
> + *
> + * This, however, has a negligible benefit compared to the increased
> + * software complexity when we would need to go through the gains
> + * for both channels separately when the integration time changes.
> + * This would end up with nasty logic for computing gain values for
> + * both channels - and rejecting them if shared bits changed.
> + *
> + * We should then build the logic by guessing what a user prefers.
> + * RGBC or IR gains correctly set while other jumps to odd value?
Ah. This is fiddly because integration time is shared. I'd missed that detail.
We could break that up, so that which they care about is dependent on which
one they wrote but it's a bit ugly.
> + * Maybe look-up a value where both gains are somehow optimized
> + * <what this somehow is, is ATM unknown to us>. Or maybe user would
> + * expect us to reject changes when optimal gains can't be set to both
> + * channels w/given integration time. At best that would result
> + * solution that works well for a very specific subset of
> + * configurations but causes unexpected corner-cases.
> + *
> + * So, we keep it simple. Always set same selector to IR and RGBC.
> + * We disallow setting IR (as I expect that most of the users are
> + * interested in RGBC). This way we can show the user that the scales
> + * for RGBC and IR channels are different (1X Vs 2X with sel 0) while
> + * still keeping the operation deterministic.
> + */
> + regval |= FIELD_PREP(BU27008_MASK_IR_GAIN_LO, sel);
> +
> + return regmap_update_bits(data->regmap, BU27008_REG_MODE_CONTROL2,
> + BU27008_MASK_RGBC_GAIN, regval);
> +}
> +
> +static int bu27008_meas_set(struct bu27008_data *data, bool enable)
> +{
> + if (enable)
> + return regmap_set_bits(data->regmap, BU27008_REG_MODE_CONTROL3,
> + BU27008_MASK_MEAS_EN);
> +
> + return regmap_clear_bits(data->regmap, BU27008_REG_MODE_CONTROL3,
> + BU27008_MASK_MEAS_EN);
Might be cleaner with regmap_update_bits()
> +}
> +static int bu27008_validate_trigger(struct iio_dev *idev,
> + struct iio_trigger *trig)
> +{
> + struct bu27008_data *data = iio_priv(idev);
> +
> + if (data->trig != trig)
Hmm. I thought we had a standard function for this, but turns out we only have
the one that validates in the other direction. Perhaps it's wroth something
generic like iio_device_validate_own_trigger which would be similar to
iio_trigger_validate_own_device in that it would use the common parentage of
the trigger and iio_dev to check the match.
> + return -EINVAL;
> +
> + return 0;
> +}
> +static int bu27008_chip_init(struct bu27008_data *data)
> +{
> + int ret;
> +
> + /* Reset */
Kind of obvious from the code, I'd drop the comment as something that might rot over
time.
> + ret = regmap_update_bits(data->regmap, BU27008_REG_SYSTEM_CONTROL,
> + BU27008_MASK_SW_RESET, BU27008_MASK_SW_RESET);
> + if (ret)
> + return dev_err_probe(data->dev, ret, "Sensor reset failed\n");
> +
> + msleep(1);
Good to state what reset time min is from datasheet.
> +
> + return ret;
> +}
> +
> +static int bu27008_set_drdy_irq(struct bu27008_data *data, bool state)
> +{
> + if (state)
> + return regmap_set_bits(data->regmap, BU27008_REG_MODE_CONTROL3,
> + BU27008_MASK_INT_EN);
> + return regmap_clear_bits(data->regmap, BU27008_REG_MODE_CONTROL3,
> + BU27008_MASK_INT_EN);
regmap_update_bits() maybe with the mask and value supplied.
> +}
> +
> +static irqreturn_t bu27008_irq_handler(int irq, void *private)
> +{
> + struct iio_dev *idev = private;
> + struct bu27008_data *data = iio_priv(idev);
> +
> + data->old_timestamp = data->timestamp;
What is old_timestamp for? Without out setting that, this
is the same as iio_pollfunc_store_time() with the timestamp
stored in a slightly difference place and always waking the thread
(which probably doesn't matter)
> + data->timestamp = iio_get_time_ns(idev);
> +
> + if (data->trigger_enabled)
> + return IRQ_WAKE_THREAD;
> +
> + return IRQ_NONE;
> +}
> +
> +static irqreturn_t bu27008_trigger_handler(int irq, void *p)
> +{
> + struct iio_poll_func *pf = p;
> + struct iio_dev *idev = pf->indio_dev;
> + struct bu27008_data *data = iio_priv(idev);
> + __le16 raw[BU27008_NUM_CHANS + NUM_U16_IN_TSTAMP];
Not immediately obvious this is correct.
I'm fairly sure it's not...
Timestamp needs to be aligned to 8 bytes. Here it's 10 bytes
in. That's why we often use a structure to make that explicit though
even then you need to force alignment of the s64 (as it would be 32 bit aligned
on x86_32 otherwise.) Note that you only actually read 4 channels so once
you've fixed the code below, this all gets simpler. Even then, you should
use a structure to make it more obviously correct for a reviewer.
> + int ret, dummy;
> +
> + memset(&raw, 0, sizeof(raw));
You'll need this after you make it a structure as suggested as then it will
have holes and memset is needed to fill them. As code stands ] = {};
would have been cleaner but that only assigns the elements of a structure
so you'll need the memset if you change to one.
> +
> + /*
> + * After some measurements, it seems reading the
> + * BU27008_REG_MODE_CONTROL3 debounces the IRQ line
'it seems' worries me. In docs? Not that we have them but would be nice
if this statement was stronger!
> + */
> + ret = regmap_read(data->regmap, BU27008_REG_MODE_CONTROL3, &dummy);
> + if (ret < 0)
> + goto err_read;
> +
> + ret = regmap_bulk_read(data->regmap, BU27008_REG_DATA0_LO, data->buffer,
> + BU27008_HW_DATA_SIZE);
> + if (ret < 0)
> + goto err_read;
> +
> + /* Red and green are always in dedicated channels. */
> + if (*idev->active_scan_mask & BIT(BU27008_RED))
> + raw[BU27008_RED] = data->buffer[BU27008_RED];
> + if (*idev->active_scan_mask & BIT(BU27008_GREEN))
> + raw[BU27008_GREEN] = data->buffer[BU27008_GREEN];
Write them in even if disabled. The IIO core will deal with any data shuffling
needed to drop any that aren't used. That's what the avail_masks stuff
is used for.
> +
> + /*
> + * We need to check the scan mask to determine which of the
> + * BLUE/CLEAR/IR are enabled so we know which channel is used to
> + * measure which data.
> + */
> + if (*idev->active_scan_mask & BIT(BU27008_BLUE)) {
> + raw[BU27008_BLUE] = data->buffer[BU27008_DATA2];
> +
> + if (*idev->active_scan_mask & BIT(BU27008_CLEAR))
> + raw[BU27008_CLEAR] = data->buffer[BU27008_DATA3];
> + } else {
> + if (*idev->active_scan_mask & BIT(BU27008_CLEAR))
> + raw[BU27008_CLEAR] = data->buffer[BU27008_DATA2];
> + }
> + if (*idev->active_scan_mask & BIT(BU27008_IR))
> + raw[BU27008_IR] = data->buffer[BU27008_DATA3];
This dance doesn't seem right. If a channel is disabled, then we should
pack the data in the the buffer passed to iio_push_to_buffers_with_timestamp()
not attempt to have it at a fixed offset. Hopefully if the scan_index etc
all match up in the right order you should be able to just do the regmap_bulk_read
directly into the buffer you are going to push out later (that buffer needs to be
a bit larger to take the timestamp.) You have a nice scan element in
your idev, but you don't use it.
> +
> + iio_push_to_buffers_with_timestamp(idev, raw, pf->timestamp);
> +err_read:
> + iio_trigger_notify_done(idev->trig);
> +
> + return IRQ_HANDLED;
> +}
> +
> +static irqreturn_t bu27008_irq_thread_handler(int irq, void *private)
> +{
> + struct iio_dev *idev = private;
> + struct bu27008_data *data = iio_priv(idev);
> + irqreturn_t ret = IRQ_NONE;
> +
> + mutex_lock(&data->mutex);
> + if (data->trigger_enabled) {
> + iio_trigger_poll_nested(data->trig);
Add a comment here on why it makes sense to hold the mutex whilst
calling this.
> + ret = IRQ_HANDLED;
> + }
> + mutex_unlock(&data->mutex);
> +
> + return ret;
> +}
> +
> +static int bu27008_buffer_preenable(struct iio_dev *idev)
> +{
> + struct bu27008_data *data = iio_priv(idev);
> + int chan_sel, ret;
> +
> + /* Configure channel selection */
> + if (*idev->active_scan_mask & BIT(BU27008_BLUE)) {
> + if (*idev->active_scan_mask & BIT(BU27008_CLEAR))
Whilst in this case active_scan_mask is a single long, you should treat
it as if it were a bitmap and use the bitmap accessors as per comments in
linux/bitmap.h
test_bit() is probably appropriate here.
> + chan_sel = BU27008_BLUE2_CLEAR3;
> + else
> + chan_sel = BU27008_BLUE2_IR3;
> + } else {
> + chan_sel = BU27008_CLEAR2_IR3;
> + }
> +
> + chan_sel = FIELD_PREP(BU27008_MASK_CHAN_SEL, chan_sel);
> +
> + ret = regmap_update_bits(data->regmap, BU27008_REG_MODE_CONTROL3,
> + BU27008_MASK_CHAN_SEL, chan_sel);
> + if (ret)
> + return ret;
> +
> + return bu27008_meas_set(data, true);
> +}
> +
> +static int bu27008_buffer_postdisable(struct iio_dev *idev)
> +{
> + struct bu27008_data *data = iio_priv(idev);
> +
> + return bu27008_meas_set(data, false);
> +}
> +
> +static const struct iio_buffer_setup_ops bu27008_buffer_ops = {
> + .preenable = bu27008_buffer_preenable,
> + .postdisable = bu27008_buffer_postdisable,
> +};
> +
> +static int bu27008_probe(struct i2c_client *i2c)
> +{
> + struct device *dev = &i2c->dev;
> + struct iio_trigger *indio_trig;
> + struct bu27008_data *data;
> + struct regmap *regmap;
> + unsigned int part_id, reg;
> + struct iio_dev *idev;
> + char *name;
> + int ret;
> +
> + if (!i2c->irq) {
> + dev_err(dev, "No IRQ configured\n");
If it's possible to relax this requirement for an IRQ you should definitely
do so, even if you loose a lot of functionality. It's annoyingly common for
board designers to think it's not necessary to wire up IRQs.
> + return -EINVAL;
> + }
> +
> + regmap = devm_regmap_init_i2c(i2c, &bu27008_regmap);
> + if (IS_ERR(regmap))
> + return dev_err_probe(dev, PTR_ERR(regmap),
> + "Failed to initialize Regmap\n");
> +
> + idev = devm_iio_device_alloc(dev, sizeof(*data));
> + if (!idev)
> + return -ENOMEM;
> +
> + ret = devm_regulator_get_enable(dev, "vdd");
> + if (ret)
> + return dev_err_probe(dev, ret, "Failed to get regulator\n");
> +
> + data = iio_priv(idev);
> +
> + ret = regmap_read(regmap, BU27008_REG_SYSTEM_CONTROL, ®);
> + if (ret)
> + return dev_err_probe(dev, ret, "Failed to access sensor\n");
> +
> + part_id = FIELD_GET(BU27008_MASK_PART_ID, reg);
> +
> + if (part_id != BU27008_ID)
> + dev_warn(dev, "unknown device 0x%x\n", part_id);
> +
> + ret = devm_iio_init_iio_gts(dev, BU27008_SCALE_1X, 0, bu27008_gains,
> + ARRAY_SIZE(bu27008_gains), bu27008_itimes,
> + ARRAY_SIZE(bu27008_itimes), &data->gts);
> + if (ret)
> + return ret;
> +
> + ret = devm_iio_init_iio_gts(dev, BU27008_SCALE_1X, 0, bu27008_gains_ir,
> + ARRAY_SIZE(bu27008_gains_ir), bu27008_itimes,
> + ARRAY_SIZE(bu27008_itimes), &data->gts_ir);
> + if (ret)
> + return ret;
> +
> + mutex_init(&data->mutex);
> + data->regmap = regmap;
> + data->dev = dev;
> + data->irq = i2c->irq;
> +
> + idev->channels = bu27008_channels;
> + idev->num_channels = ARRAY_SIZE(bu27008_channels);
> + idev->name = "bu27008";
> + idev->info = &bu27008_info;
> + idev->modes = INDIO_DIRECT_MODE | INDIO_BUFFER_SOFTWARE;
Should be INDIO_BUFFER_TRIGGERRED I think. which is set by
devm_iio_triggered_buffer_setup() for you. So just DIRECT_MODE here.
> + idev->available_scan_masks = bu27008_scan_masks;
> +
> + ret = bu27008_chip_init(data);
> + if (ret)
> + return ret;
Blank line here to separate the blocks of connected code from those that
are unrelated.
> + ret = devm_iio_triggered_buffer_setup_ext(dev, idev,
> + &iio_pollfunc_store_time,
> + bu27008_trigger_handler,
> + IIO_BUFFER_DIRECTION_IN,
> + &bu27008_buffer_ops,
> + NULL);
I'm guessing this is left from a point in development where last parameter wasn't NULL.
Given it is you can just use the more common
devm_iio_triggered_buffer_setup() that also assumes it's an input buffers (which this is)
> + if (ret)
> + return dev_err_probe(data->dev, ret,
> + "iio_triggered_buffer_setup_ext FAIL\n");
> +
> + indio_trig = devm_iio_trigger_alloc(dev, "%sdata-rdy-dev%d", idev->name,
> + iio_device_id(idev));
Bit of a mix of naming conventions in here. Both occur in various drivers, but
not normally mixed up. Either
idev, itrig
or
indio_dev, indio_trig
> + if (!indio_trig)
> + return -ENOMEM;
> +
> + data->trig = indio_trig;
> +
> + indio_trig->ops = &bu27008_trigger_ops;
> + iio_trigger_set_drvdata(indio_trig, data);
> +
> + /*
> + * No need to check for NULL. request_threaded_irq() defaults to
> + * dev_name() should the alloc fail.
Whilst true, it's very unlikely we'll get a failure here and
the code to check is shorter than the comment. Hence I'd just check and error
out on failure anyway in interests of readability and simplicity.
> + */
> + name = devm_kasprintf(data->dev, GFP_KERNEL, "%s-bu27008",
> + dev_name(data->dev));
> +
> + ret = devm_request_threaded_irq(data->dev, i2c->irq, bu27008_irq_handler,
(dev, ...
in all of these.
> + &bu27008_irq_thread_handler,
> + IRQF_ONESHOT, name, idev);
> + if (ret)
> + return dev_err_probe(data->dev, ret, "Could not request IRQ\n");
> +
> +
> + ret = devm_iio_trigger_register(dev, indio_trig);
> + if (ret)
> + return dev_err_probe(data->dev, ret,
Be consistent on data->dev vs dev. I'd prefer dev as it's shorter.
> + "Trigger registration failed\n");
> +
> + ret = devm_iio_device_register(data->dev, idev);
> + if (ret < 0)
if (ret) for consistency and as it's documented as never taking a postive value
anyway.
> + return dev_err_probe(dev, ret,
> + "Unable to register iio device\n");
> +
> + return ret;
> +}
On 4/23/23 15:04, Jonathan Cameron wrote:
> On Fri, 21 Apr 2023 12:37:30 +0300
> Matti Vaittinen <[email protected]> wrote:
>
>> Add support for ROHM BU27008 RGB sensor.
>>
>> The ROHM BU27008 is a sensor with 5 photodiodes (red, green, blue, clear
>> and IR) with four configurable channels. Red and green being always
>> available and two out of the rest three (blue, clear, IR) can be
>> selected to be simultaneously measured. Typical application is adjusting
>> LCD backlight of TVs, mobile phones and tablet PCs.
>>
>> This series supports reading the RGBC and IR channels using IIO
>> frameeork. However, only two of the BC+IR can be enabled at the same
>
> framework
Thanks!
>
>> time. Series adds also support for scale and integration time
>> configuration, where scale consists of impact of both the integration
>> time and hardware gain. The gain and time support is backed by the newly
>> introduced IIO GTS helper. This series depends on GTS helper patches
>> added in BU27034 support series which is already merged in iio/togreg
>> which this series is based on.
>>
>> The hardware allows configuring gain setting by writing a 5-bit gain
>> selector value to a register. Part of the gain setting is common for all
>> channels (RGBC + IR) but part of the selector value can be set
>> separately for RGBC and IR:
>>
>> MODE_CONTROL2 REG:
>> bit 7 6 5 4 3 2 1 0
>> -----------------------------------------------------------------
>> | RGB selector |
>> +---------------------------------------+
>> -----------------------------------------------------------------
>> | high bits IR | | low bits IR selector |
>> +---------------+ +-----------------------+
>>
>> In theory it would be possible to set certain separate gain values for
>> RGBC and IR channels, but this gets pretty confusing because there are a
>> few 'unsupported' selector values. If only RGBC or IR was set, some
>> extra handling should be done to prevent the other channel from getting
>> unsupported value due to change in high-bits. Furthermore, allowing the
>> channels to be set different gain values (in some cases when gains are
>> such the HW supports it) would make the cases where also integration
>> time is changed to achieve correct scale ... interesting. It might also
>> be confusing for user to try predicting when setting different scales
>> succeeds and when it does not. Furthermore, if for example the scale
>> setting for RGBC caused IR selector to be invalid - it could also cause
>> the IR scale to "jump" very far from previous value.
>>
>> To make the code simpler and more predictable for users, the current
>> logic is as follows:
>>
>> 1. Prevent setting IR scale. (My assumption is IR is less used than
>> RGBC)
>> 2. When RGBC scale is set, set also the IR-selector to the same value.
>> This prevents unsupported selector values and makes the IR scale changes
>> predictable.
>>
>> The 2) could mean we effectively have the same scale for all channels.
>> Unfortunately, the HW design is slightly peculiar and selector 0 means
>> gain 1X on RGBC but gain 2X on IR. Rest of the selectors equal same gain
>> values on RGBC and IR. The result is that while changin selector from 0
>> => 1 causes RGBC gain to go from 1X => 4X, it causes IR gain to go from
>> 2X => 4X.
>>
>> So, the driver provides separate scale entries for all channels (also
>> RGB and C will have separate gain entries because these channels are of
>> same type as IR channel). This makes it possible for user applications
>> to go read the scales for all channels after setting scale for one (in
>> order to detect the IR scale difference).
>>
>> Having the separate IR scale entry which applications can read to detect
>> "arbitrary scale changes" makes it possible for applications to be
>> written so they can cope if we need to implement the 'allow setting some
>> different gains for IR and RGBC' - later.
>
> Hmm. I'm not sure preventing it is the best approach. That makes for an
> interface that is perhaps even less intuitive than having it affect the
> scales of the other channels. Still having it configurable at all is
> optional from an ABI point of view, so we could go with what you have
> here and see if anyone shouts about it in future.
I am not sure either. This is really a dance between making driver which
is still understandable and maintainable, ABI which allows us to improve
things if needed and providing users the features they need.
So. let's begin with this - and as you say, if it appears to be
suboptimal, then we can re-work it :)
Yours,
-- Matti
--
Matti Vaittinen
Linux kernel developer at ROHM Semiconductors
Oulu Finland
~~ When things go utterly wrong vim users can always type :help! ~~
On 4/23/23 15:57, Jonathan Cameron wrote:
> On Fri, 21 Apr 2023 12:39:36 +0300
> Matti Vaittinen <[email protected]> wrote:
>
>> The ROHM BU27008 is a sensor with 5 photodiodes (red, green, blue, clear
>> and IR) with four configurable channels. Red and green being always
>> available and two out of the rest three (blue, clear, IR) can be
>> selected to be simultaneously measured. Typical application is adjusting
>> LCD backlight of TVs, mobile phones and tablet PCs.
>>
>> Add initial support for the ROHM BU27008 color sensor.
>> - raw_read() of RGB and clear channels
>> - triggered buffer w/ DRDY interrtupt
>>
>> Signed-off-by: Matti Vaittinen <[email protected]>
>
> Hi Matti,
>
> Biggest issue in here is some confusion over data packing when some channels
> are enabled. The driver must pack the channels that are enabled (seen
> from active_scan_mask) according to general IIO rules. So naturally aligned
> buffers. Thus for this device it should always be packed into
>
> struct scan {
> __le16 chans[4];
> s64 ts __aligned(8); /*it's aligned anyway but better not to make reviewers think ;) */
> };
>
> Even though there are 5 possible channels. If one in the middle isnt' enabled (e.g. blue)
> then clear and IR shift to lower words. of the buffer.
Ah, right. So I had misunderstood how the buffer works. I thought the
scan_mask was only used to disallow unsupported channel-enabling
configurations. If I understand your statement correctly, the scan_mask
is used to determine the 'place of data' in the buffer when certain
configuration is used. (I'll check this from the code but if the IIO
handles data as I now think - that's cool! It should indeed simplify the
buffer in driver side!).
>
> The demux code in the IIO core then deals with userspace requesting less than this
> by repacking the data if needed. That allows it to present different views to different
> consumers (e.g. userspace IIO access, and an inkernel buffer user - though there aren't
> any of those for light sensors AFAIK.
>
> I'd not thought about the issue of the weird scales interacting with someone changing
> the integration time. That's nasty and I don't have a better suggestion than what you have
> currently.
>
> Otherwise some trivial stuff inline.
Thanks for the non-trivial and trivial stuff! I'll re-work this driver
based on your input - which is really invaluable - during this week.
Your support is _very much_ appreciated!
>
> Jonathan
>
>
>
>> diff --git a/drivers/iio/light/rohm-bu27008.c b/drivers/iio/light/rohm-bu27008.c
>> new file mode 100644
>> index 000000000000..6fca193eeb9e
>> --- /dev/null
>> +++ b/drivers/iio/light/rohm-bu27008.c
>> @@ -0,0 +1,1028 @@
>
> ...
>
>> +
>> +enum {
>> + BU27008_RED, /* Always data0 */
>> + BU27008_GREEN, /* Always data1 */
>> + BU27008_BLUE, /* data2, configurable (blue / clear) */
>> + BU27008_CLEAR, /* data2 or data3 */
>> + BU27008_IR, /* data3 */
>> + BU27008_NUM_CHANS
>> +};
>> +
>> +enum {
>> + BU27008_DATA0, /* Always RED */
>> + BU27008_DATA1, /* Always GREEN */
>> + BU27008_DATA2, /* Blue or Clear */
>> + BU27008_DATA3, /* IR or Clear */
>> + BU27008_NUM_HW_CHANS
>> +};
>> +
>> +/* We can always measure red and green at same time */
>> +#define ALWAYS_SCANNABLE (BIT(BU27008_RED) | BIT(BU27008_GREEN))
>> +
>> +#define BU27008_CHAN_DATA_SIZE 2 /* Each channel has 16bits of data */
>> +#define BU27008_BUF_DATA_SIZE (BU27008_NUM_CHANS * BU27008_CHAN_DATA_SIZE)
>
> Buffer should be same size as the number you can grab in one read. So
> it should be same as BU27008_HW_DATA_SIZE. The data is packed when only some
> channels are enabled.
>
>> +#define BU27008_HW_DATA_SIZE (BU27008_NUM_HW_CHANS * BU27008_CHAN_DATA_SIZE)
>> +#define NUM_U16_IN_TSTAMP (sizeof(s64) / sizeof(u16))
>> +
>> +static const unsigned long bu27008_scan_masks[] = {
>> + ALWAYS_SCANNABLE | BIT(BU27008_CLEAR) | BIT(BU27008_IR),
>> + ALWAYS_SCANNABLE | BIT(BU27008_CLEAR) | BIT(BU27008_BLUE),
>> + ALWAYS_SCANNABLE | BIT(BU27008_BLUE) | BIT(BU27008_IR),
>
> Good, this looks correct for allowing the IIO core to deal with demuxing
> the data as necessary. Note that this is saying that if one of these
> modes is active scan mask it will be backed into lowers set of __le16
> buffer elements.
So basically, what I need to ensure is that the data in the buffer will
be in the same order we tell here for "IIO demuxer". I think I got this
now, thanks!
>
>
>> +static int bu27008_validate_trigger(struct iio_dev *idev,
>> + struct iio_trigger *trig)
>> +{
>> + struct bu27008_data *data = iio_priv(idev);
>> +
>> + if (data->trig != trig)
>
> Hmm. I thought we had a standard function for this, but turns out we only have
> the one that validates in the other direction. Perhaps it's wroth something
> generic like iio_device_validate_own_trigger which would be similar to
> iio_trigger_validate_own_device in that it would use the common parentage of
> the trigger and iio_dev to check the match.
I'll see if I can come up with something generic based on your suggestion :)
>> + ret = regmap_update_bits(data->regmap, BU27008_REG_SYSTEM_CONTROL,
>> + BU27008_MASK_SW_RESET, BU27008_MASK_SW_RESET);
>> + if (ret)
>> + return dev_err_probe(data->dev, ret, "Sensor reset failed\n");
>> +
>> + msleep(1);
>
> Good to state what reset time min is from datasheet.
I don't remember seeing that in datasheet, but I'll recheck.
>> +
>> + /*
>> + * After some measurements, it seems reading the
>> + * BU27008_REG_MODE_CONTROL3 debounces the IRQ line
>
> 'it seems' worries me. In docs? Not that we have them but would be nice
> if this statement was stronger!
I have the - or at least a - datasheet. Unfortunately the datasheet does
not bother explaining too much about how things are working... The
'seems' here is based on my observation that without reading the
BU27008_REG_MODE_CONTROL3 all the samples are read immediately
regardless the integration time. Also, tracing the lines with saleae
does not show the IRQ line being debounced when samples are read. This
behaviour changes when I read the BU27008_REG_MODE_CONTROL3 - which
contains the 'VALID'-bit. Also saleae shows the IRQ debounced when this
reg is read. Hence, I am fairly convinced the IC works like this - and I
am somewhat convinced the IC is meant to work like this - but no, I
haven't seen this documented (nor did I receive confirmation from the
hardware colleagues when I asked about this). So, this comment just
states my current understanding - HW 'seems' to work like this :)
>> +
>> + if (!i2c->irq) {
>> + dev_err(dev, "No IRQ configured\n");
>
> If it's possible to relax this requirement for an IRQ you should definitely
> do so, even if you loose a lot of functionality. It's annoyingly common for
> board designers to think it's not necessary to wire up IRQs.
Well, knowing how IC designers implement the IRQs... I am not at all
sure not wiring the IRQs is a bad thing XD
And yes, a light sensor is probably very much usable with plain polling.
I'll just drop the buffer/trigger stuff and provide only the
read/write_raw() if IRQ is not given.
All in all, thanks a lot for the review!
Yours,
-- Matti
--
Matti Vaittinen
Linux kernel developer at ROHM Semiconductors
Oulu Finland
~~ When things go utterly wrong vim users can always type :help! ~~
On 4/23/23 15:57, Jonathan Cameron wrote:
> On Fri, 21 Apr 2023 12:39:36 +0300
> Matti Vaittinen <[email protected]> wrote:
>
>> The ROHM BU27008 is a sensor with 5 photodiodes (red, green, blue, clear
>> and IR) with four configurable channels. Red and green being always
>> available and two out of the rest three (blue, clear, IR) can be
>> selected to be simultaneously measured. Typical application is adjusting
>> LCD backlight of TVs, mobile phones and tablet PCs.
>>
>> Add initial support for the ROHM BU27008 color sensor.
>> - raw_read() of RGB and clear channels
>> - triggered buffer w/ DRDY interrtupt
>>
>> Signed-off-by: Matti Vaittinen <[email protected]>
>> +
>> +static int bu27008_meas_set(struct bu27008_data *data, bool enable)
>> +{
>> + if (enable)
>> + return regmap_set_bits(data->regmap, BU27008_REG_MODE_CONTROL3,
>> + BU27008_MASK_MEAS_EN);
>> +
>> + return regmap_clear_bits(data->regmap, BU27008_REG_MODE_CONTROL3,
>> + BU27008_MASK_MEAS_EN);
>
> Might be cleaner with regmap_update_bits()
>
>> +}
Hm. I need to disagree on this although I think it depends on what one
is used to :)
For me adding a variable for value to be used is slightly more complex
than just using clear or set function depending on the enable/disable. I
remember thinking the same as you and preferring the update_bits also on
enable/disable cases - until I wrote my first power-supply driver and
Sebasian Reichel told me to not do:
int val;
if (foo)
val = mask;
else
val = 0;
return regmap_update_bits(regmap, reg, mask, val);
but use set/clear bits. This allows killing the 'int val;'. I remember I
had to sleep over night on it but I later started seeing the set/clear
bits as a simpler thing.
Sure we could also do
if (foo)
return regmap_update_bits(map, reg, mask, mask);
else
return regmap_update_bits(map, reg, mask, 0);
- but here we just replace:
regmap_set_bits(map, reg, mask) with
regmap_update_bits(map, reg, mask, mask)
and
regmap_clear_bits(map, reg, mask)
regmap_update_bits(map, reg, mask, 0)
with longer but functionally same variants - which kind of says "I think
the "regmap_set_bits() and regmap_clear_bits()" are useless ;)
After saying this - I can use the regmap_update_bits() if you insist,
but in my (not always so) humble opinion this does not improve the function.
>> +
>> +static int bu27008_set_drdy_irq(struct bu27008_data *data, bool state)
>> +{
>> + if (state)
>> + return regmap_set_bits(data->regmap, BU27008_REG_MODE_CONTROL3,
>> + BU27008_MASK_INT_EN);
>> + return regmap_clear_bits(data->regmap, BU27008_REG_MODE_CONTROL3,
>> + BU27008_MASK_INT_EN);
> regmap_update_bits() maybe with the mask and value supplied.
Same weak objection here as was with the bu27008_meas_set(). Eg, can
change if required but please reconsider :)
>> +}
>> +
>
>> +static irqreturn_t bu27008_irq_handler(int irq, void *private)
>> +{
>> + struct iio_dev *idev = private;
>> + struct bu27008_data *data = iio_priv(idev);
>> +
>> + data->old_timestamp = data->timestamp;
>
> What is old_timestamp for? Without out setting that, this
> is the same as iio_pollfunc_store_time() with the timestamp
> stored in a slightly difference place and always waking the thread
> (which probably doesn't matter)
Thanks. I just re-used the logic from a driver which had some other
options but the data-ready IRQ as well. As we don't have any such in
bu27008, I think we can drop the custom stuff as you suggest and clean
up this for quite a bit :) Thanks!
>> +
>> +static irqreturn_t bu27008_irq_thread_handler(int irq, void *private)
>> +{
>> + struct iio_dev *idev = private;
>> + struct bu27008_data *data = iio_priv(idev);
>> + irqreturn_t ret = IRQ_NONE;
>> +
>> + mutex_lock(&data->mutex);
>> + if (data->trigger_enabled) {
>> + iio_trigger_poll_nested(data->trig);
>
> Add a comment here on why it makes sense to hold the mutex whilst
> calling this.
After revising this - I don't think it makes. Nor do I think we need the
trigger_enable flag so we don't propably need the mutex in buffer enable
either as all raw-write configs are claiming the direct mode.
I'll cook the v2 soon(ish). Thanks!
Yours,
--Matti
--
Matti Vaittinen
Linux kernel developer at ROHM Semiconductors
Oulu Finland
~~ When things go utterly wrong vim users can always type :help! ~~
On Mon, 24 Apr 2023 13:14:56 +0300
Matti Vaittinen <[email protected]> wrote:
> On 4/23/23 15:57, Jonathan Cameron wrote:
> > On Fri, 21 Apr 2023 12:39:36 +0300
> > Matti Vaittinen <[email protected]> wrote:
> >
> >> The ROHM BU27008 is a sensor with 5 photodiodes (red, green, blue, clear
> >> and IR) with four configurable channels. Red and green being always
> >> available and two out of the rest three (blue, clear, IR) can be
> >> selected to be simultaneously measured. Typical application is adjusting
> >> LCD backlight of TVs, mobile phones and tablet PCs.
> >>
> >> Add initial support for the ROHM BU27008 color sensor.
> >> - raw_read() of RGB and clear channels
> >> - triggered buffer w/ DRDY interrtupt
> >>
> >> Signed-off-by: Matti Vaittinen <[email protected]>
> >> +
> >> +static int bu27008_meas_set(struct bu27008_data *data, bool enable)
> >> +{
> >> + if (enable)
> >> + return regmap_set_bits(data->regmap, BU27008_REG_MODE_CONTROL3,
> >> + BU27008_MASK_MEAS_EN);
> >> +
> >> + return regmap_clear_bits(data->regmap, BU27008_REG_MODE_CONTROL3,
> >> + BU27008_MASK_MEAS_EN);
> >
> > Might be cleaner with regmap_update_bits()
> >
> >> +}
>
> Hm. I need to disagree on this although I think it depends on what one
> is used to :)
>
> For me adding a variable for value to be used is slightly more complex
> than just using clear or set function depending on the enable/disable. I
> remember thinking the same as you and preferring the update_bits also on
> enable/disable cases - until I wrote my first power-supply driver and
> Sebasian Reichel told me to not do:
>
> int val;
>
> if (foo)
> val = mask;
> else
> val = 0;
>
> return regmap_update_bits(regmap, reg, mask, val);
>
> but use set/clear bits. This allows killing the 'int val;'. I remember I
> had to sleep over night on it but I later started seeing the set/clear
> bits as a simpler thing.
>
> Sure we could also do
>
> if (foo)
> return regmap_update_bits(map, reg, mask, mask);
> else
> return regmap_update_bits(map, reg, mask, 0);
>
> - but here we just replace:
>
> regmap_set_bits(map, reg, mask) with
> regmap_update_bits(map, reg, mask, mask)
>
> and
>
> regmap_clear_bits(map, reg, mask)
> regmap_update_bits(map, reg, mask, 0)
>
> with longer but functionally same variants - which kind of says "I think
> the "regmap_set_bits() and regmap_clear_bits()" are useless ;)
>
> After saying this - I can use the regmap_update_bits() if you insist,
> but in my (not always so) humble opinion this does not improve the function.
Makes sense. Leave it as it stands.
>
>
> >> +
> >> +static int bu27008_set_drdy_irq(struct bu27008_data *data, bool state)
> >> +{
> >> + if (state)
> >> + return regmap_set_bits(data->regmap, BU27008_REG_MODE_CONTROL3,
> >> + BU27008_MASK_INT_EN);
> >> + return regmap_clear_bits(data->regmap, BU27008_REG_MODE_CONTROL3,
> >> + BU27008_MASK_INT_EN);
> > regmap_update_bits() maybe with the mask and value supplied.
>
> Same weak objection here as was with the bu27008_meas_set(). Eg, can
> change if required but please reconsider :)
Sure. Was a 'maybe' :)
>
> >> +}
> >> +
> >> +
> >> +static irqreturn_t bu27008_irq_thread_handler(int irq, void *private)
> >> +{
> >> + struct iio_dev *idev = private;
> >> + struct bu27008_data *data = iio_priv(idev);
> >> + irqreturn_t ret = IRQ_NONE;
> >> +
> >> + mutex_lock(&data->mutex);
> >> + if (data->trigger_enabled) {
> >> + iio_trigger_poll_nested(data->trig);
> >
> > Add a comment here on why it makes sense to hold the mutex whilst
> > calling this.
>
> After revising this - I don't think it makes. Nor do I think we need the
> trigger_enable flag so we don't propably need the mutex in buffer enable
> either as all raw-write configs are claiming the direct mode.
Clearing this out meant I noticed the oddity of doing this in the thread
at all. So all good in the end ;)
Jonathan
On Mon, 24 Apr 2023 06:21:23 +0000
"Vaittinen, Matti" <[email protected]> wrote:
> On 4/23/23 15:57, Jonathan Cameron wrote:
> > On Fri, 21 Apr 2023 12:39:36 +0300
> > Matti Vaittinen <[email protected]> wrote:
> >
> >> The ROHM BU27008 is a sensor with 5 photodiodes (red, green, blue, clear
> >> and IR) with four configurable channels. Red and green being always
> >> available and two out of the rest three (blue, clear, IR) can be
> >> selected to be simultaneously measured. Typical application is adjusting
> >> LCD backlight of TVs, mobile phones and tablet PCs.
> >>
> >> Add initial support for the ROHM BU27008 color sensor.
> >> - raw_read() of RGB and clear channels
> >> - triggered buffer w/ DRDY interrtupt
> >>
> >> Signed-off-by: Matti Vaittinen <[email protected]>
> >
> > Hi Matti,
> >
> > Biggest issue in here is some confusion over data packing when some channels
> > are enabled. The driver must pack the channels that are enabled (seen
> > from active_scan_mask) according to general IIO rules. So naturally aligned
> > buffers. Thus for this device it should always be packed into
> >
> > struct scan {
> > __le16 chans[4];
> > s64 ts __aligned(8); /*it's aligned anyway but better not to make reviewers think ;) */
> > };
> >
> > Even though there are 5 possible channels. If one in the middle isnt' enabled (e.g. blue)
> > then clear and IR shift to lower words. of the buffer.
>
> Ah, right. So I had misunderstood how the buffer works. I thought the
> scan_mask was only used to disallow unsupported channel-enabling
> configurations. If I understand your statement correctly, the scan_mask
> is used to determine the 'place of data' in the buffer when certain
> configuration is used. (I'll check this from the code but if the IIO
> handles data as I now think - that's cool! It should indeed simplify the
> buffer in driver side!).
'Place' is a little confusing (English is imprecise sometimes). Order
is perhaps more precise.
Place can mean same as order - as in 1st place in a race, but it can
also mean a specific location such as a place at a table where only
some seats are full.
The handling for this came about as part of the multiple consumer
support for SoC ADCs but it was also useful for ripping out a whole
load of driver specific handling that did similar repacking and letting
the generic code handle repacking data. It is fairly optimal and
does things like larger memcpys if there are a set of channels
that need moving, and cleanly makes it a noop if there is nothing
to do at all. All those tricks came IIRC from individual drivers
that had previously been doing this magic.
Jonathan