Re: [PATCH v6 2/2] iio: light: ROHM BH1745 colour sensor
From: Mudit Sharma
Date: Mon Jul 01 2024 - 17:33:10 EST
On 01/07/2024 12:32, Matti Vaittinen wrote:
+
+// From 16x max HW gain and 32x max integration time
+#define BH1745_MAX_GAIN 512
+
+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 };
+
+static const int bh1745_int_time_us[] = { 160000, 320000, 640000,
+ 1280000, 2560000, 5120000 };
I am not sure why you need these tables above? Can't the iio_gts do all
the conversions from register-value to int time/gain and int-time/gain
to register value, as well as the checks for supported values? Ideally,
you would not need anything else but the bh1745_itimes and the
bh1745_gain tables below - they should contain all the same information.
Hi Matti,
Thank you for reviewing this.
Just had a look again at GTS helpers and found the appropriate
functions. Will drop these for v7.
+};
+
+static const struct regmap_range bh1745_volatile_ranges[] = {
+ regmap_reg_range(BH1745_MODE_CTRL_2, BH1745_MODE_CTRL_2), /*
VALID */
+ regmap_reg_range(BH1745_RED_LSB, BH1745_CLEAR_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_RED_LSB, BH1745_CLEAR_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_INTR, BH1745_INTR),
+ 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,
I am not 100% sure what this does. (Let's say it is just my ignorance
:)). Does the 'ro' in 'bh1745_ro_regs' stand for read-only?
If so, shouldn't the read-inly registers be marked as "not writable",
which would be adding them in .wr_table in 'no_ranges'? Also, what is
the idea of the 'wr_regs'?
There are some read-only (Manufacturer ID and Data registers) and some
readable and writable registers. I will rename these to
'bh1745_readable_regs' and 'bh1745_writable_regs' in v7 to avoid confusion.
+};
+
+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),
+ .mask_separate = BIT(IIO_EV_INFO_ENABLE),
+ },
+};
+
+#define BH1745_CHANNEL(_colour, _si,
_addr) \
+ { \
+ .type = IIO_INTENSITY, .modified = 1, \
+ .info_mask_separate = BIT(IIO_CHAN_INFO_RAW), \
+ .info_mask_shared_by_all = BIT(IIO_CHAN_INFO_SCALE) | \
+ BIT(IIO_CHAN_INFO_INT_TIME), \
+ .info_mask_shared_by_all_available = \
+ BIT(IIO_CHAN_INFO_SCALE) | \
+ 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_RED_LSB),
+ BH1745_CHANNEL(GREEN, 1, BH1745_GREEN_LSB),
+ BH1745_CHANNEL(BLUE, 2, BH1745_BLUE_LSB),
+ BH1745_CHANNEL(CLEAR, 3, BH1745_CLEAR_LSB),
+ IIO_CHAN_SOFT_TIMESTAMP(4),
+};
+
+static int bh1745_reset(struct bh1745_data *data)
+{
+ int ret;
+ int value;
+
+ ret = regmap_read(data->regmap, BH1745_SYS_CTRL, &value);
+ if (ret)
+ return ret;
+
+ value |= (BH1745_SW_RESET | BH1745_INT_RESET);
+
+ return regmap_write(data->regmap, BH1745_SYS_CTRL, value);
Would it work if you used regmap_write_bits() instead?
Yes, it should work. Jonathan suggested 'regmap_set_bits()' for this and
it looks like a good fit for this as well.
I will look through the regmap API once again and update similar cases.
... Sorry, my reviewing time is out :/ I may continue later but no need
to wait for my comments if I am not responding. I've too much stuff
piling on :(
Yours,
-- Matti
Best regards,
Mudit Sharma