Re: [PATCH v6 2/2] iio: light: ROHM BH1745 colour sensor

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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




[Index of Archives]     [Device Tree Compilter]     [Device Tree Spec]     [Linux Driver Backports]     [Video for Linux]     [Linux USB Devel]     [Linux PCI Devel]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [XFree86]     [Yosemite Backpacking]


  Powered by Linux