| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <3ececd1c-8331-4007-8e75-a000425ccd3c@gmail.com>
Date: Mon, 1 Jul 2024 22:32:55 +0100
From: Mudit Sharma <muditsharma.info@...il.com>
To: Matti Vaittinen <mazziesaccount@...il.com>, jic23@...nel.org,
lars@...afoo.de, krzk+dt@...nel.org, conor+dt@...nel.org, robh@...nel.org
Cc: linux-kernel@...r.kernel.org, linux-iio@...r.kernel.org,
devicetree@...r.kernel.org, Ivan Orlov <ivan.orlov0322@...il.com>,
Javier Carrasco <javier.carrasco.cruz@...il.com>
Subject: Re: [PATCH v6 2/2] iio: light: ROHM BH1745 colour sensor
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
Powered by blists - more mailing lists