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.
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 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'?
+}; + +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
