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

[Mudit Sharma <muditsharma.info@gmail.com>]

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