| 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: <CAHp75VeaYBGTA7sN7SefsyMj09kaJLBoMz4=hf0GpxiXtF65+Q@mail.gmail.com>
Date: Sat, 5 Oct 2024 21:53:51 +0300
From: Andy Shevchenko <andy.shevchenko@...il.com>
To: Antoniu Miclaus <antoniu.miclaus@...log.com>
Cc: Jonathan Cameron <jic23@...nel.org>, Lars-Peter Clausen <lars@...afoo.de>,
Michael Hennerich <Michael.Hennerich@...log.com>, Rob Herring <robh@...nel.org>,
Krzysztof Kozlowski <krzk+dt@...nel.org>, Conor Dooley <conor+dt@...nel.org>, Nuno Sa <nuno.sa@...log.com>,
Olivier Moysan <olivier.moysan@...s.st.com>, Uwe Kleine-König <ukleinek@...nel.org>,
Andy Shevchenko <andy@...nel.org>, David Lechner <dlechner@...libre.com>,
Marcelo Schmitt <marcelo.schmitt@...log.com>, Mike Looijmans <mike.looijmans@...ic.nl>,
Marius Cristea <marius.cristea@...rochip.com>, Dumitru Ceclan <mitrutzceclan@...il.com>,
AngeloGioacchino Del Regno <angelogioacchino.delregno@...labora.com>,
Alisa-Dariana Roman <alisadariana@...il.com>, Ivan Mikhaylov <fr0st61te@...il.com>,
Sergiu Cuciurean <sergiu.cuciurean@...log.com>, Dragos Bogdan <dragos.bogdan@...log.com>,
linux-iio@...r.kernel.org, linux-kernel@...r.kernel.org,
devicetree@...r.kernel.org, linux-pwm@...r.kernel.org
Subject: Re: [PATCH v2 6/7] iio: adc: ad485x: add ad485x driver
On Fri, Oct 4, 2024 at 5:12 PM Antoniu Miclaus
<antoniu.miclaus@...log.com> wrote:
>
> Add support for the AD485X DAS familiy.
family.
...
> +#define AD485X_REG_PRODUCT_ID_L 0x04
> +#define AD485X_REG_PRODUCT_ID_H 0x05
Can this use bulk transfer with __le16 type?
...
> +#define AD485X_REG_CHX_OFFSET_LSB(ch) AD485X_REG_CHX_OFFSET(ch)
> +#define AD485X_REG_CHX_OFFSET_MID(ch) (AD485X_REG_CHX_OFFSET_LSB(ch) + 0x01)
> +#define AD485X_REG_CHX_OFFSET_MSB(ch) (AD485X_REG_CHX_OFFSET_MID(ch) + 0x01)
But can you connect oscilloscope and actually see what's the difference?
...
> +#define AD485X_REG_CHX_GAIN_LSB(ch) AD485X_REG_CHX_GAIN(ch)
> +#define AD485X_REG_CHX_GAIN_MSB(ch) (AD485X_REG_CHX_GAIN(ch) + 0x01)
> +#define AD485X_REG_CHX_PHASE_LSB(ch) AD485X_REG_CHX_PHASE(ch)
> +#define AD485X_REG_CHX_PHASE_MSB(ch) (AD485X_REG_CHX_PHASE_LSB(ch) + 0x01)
__le16 + bulk transfers?
...
> +#define AD485X_SW_RESET (BIT(7) | BIT(0))
Is it really a bitfield? What then does each bit mean?
> +#define AD485X_SDO_ENABLE BIT(4)
> +#define AD485X_SINGLE_INSTRUCTION BIT(7)
> +#define AD485X_ECHO_CLOCK_MODE BIT(0)
...
> +struct ad485x_chip_info {
> + const char *name;
> + unsigned int product_id;
> + const unsigned int (*scale_table)[2];
> + int num_scales;
> + const int *offset_table;
> + int num_offset;
> + const struct iio_chan_spec *channels;
> + unsigned int num_channels;
> + unsigned long throughput;
> + unsigned int resolution;
Have you run `pahole` for this and other data structures you
introduced? Is there any room for optimization?
> +};
...
> +static int ad485x_find_opt(bool *field, u32 size, u32 *ret_start)
> +{
> + unsigned int i, cnt = 0, max_cnt = 0, max_start = 0;
> + int start;
> +
> + for (i = 0, start = -1; i < size; i++) {
> + if (field[i] == 0) {
> + if (start == -1)
> + start = i;
> + cnt++;
> + } else {
> + if (cnt > max_cnt) {
> + max_cnt = cnt;
> + max_start = start;
> + }
> + start = -1;
> + cnt = 0;
> + }
> + }
> +
> + if (cnt > max_cnt) {
> + max_cnt = cnt;
> + max_start = start;
> + }
> +
> + if (!max_cnt)
> + return -ENOENT;
> +
> + *ret_start = max_start;
> +
> + return max_cnt;
> +}
Can you describe the search algorithm in the comment?
...
> +static int ad485x_calibrate(struct ad485x_state *st)
> +{
> + unsigned int opt_delay, lane_num, delay, i, s, c;
> + enum iio_backend_interface_type interface_type;
> + bool pn_status[AD485X_MAX_LANES][AD485X_MAX_IODELAY];
Why bool and not bitmap? I think I already asked this, but I don't
remember what you answered.
> + int ret;
...
> +static int ad485x_set_packet_format(struct iio_dev *indio_dev,
> + const struct iio_chan_spec *chan,
> + unsigned int format)
> +{
> + struct ad485x_state *st = iio_priv(indio_dev);
> + unsigned int val;
> + int ret;
> +
> + if (chan->scan_type.realbits == 20)
Missing {}
> + switch (format) {
> + case 0:
> + val = 20;
> + break;
> + case 1:
> + val = 24;
> + break;
> + case 2:
> + val = 32;
> + break;
> + default:
> + return -EINVAL;
> + }
> + else if (chan->scan_type.realbits == 16)
Ditto.
> + switch (format) {
> + case 0:
> + val = 16;
> + break;
> + case 1:
> + val = 24;
> + break;
> + default:
> + return -EINVAL;
> + }
> + else
Ditto.
> + return -EINVAL;
> +
> + ret = iio_backend_data_size_set(st->back, val);
> + if (ret)
> + return ret;
> +
> + return regmap_update_bits(st->regmap, AD485X_REG_PACKET,
> + AD485X_PACKET_FORMAT_MASK, format);
> +}
...
> +static int ad485x_get_calibscale(struct ad485x_state *st, int ch, int *val, int *val2)
> +{
> + unsigned int reg_val;
> + int gain;
Should be u8 gain[2] and...
> + int ret;
> +
> + guard(mutex)(&st->lock);
> +
> + ret = regmap_read(st->regmap, AD485X_REG_CHX_GAIN_MSB(ch),
> + ®_val);
> + if (ret)
> + return ret;
> +
> + gain = (reg_val & 0xFF) << 8;
> +
> + ret = regmap_read(st->regmap, AD485X_REG_CHX_GAIN_LSB(ch),
> + ®_val);
> + if (ret)
> + return ret;
> +
> + gain |= reg_val & 0xFF;
> + *val = gain;
...get_unaligned_be/le16().
> + *val2 = 32768;
> +
> + return IIO_VAL_FRACTIONAL;
> +}
...
> +static int ad485x_set_calibscale(struct ad485x_state *st, int ch, int val,
> + int val2)
> +{
> + unsigned long long gain;
> + unsigned int reg_val;
> + int ret;
> + gain = val * MICRO + val2;
> + gain = DIV_U64_ROUND_CLOSEST(gain * 32768, MICRO);
> +
> + reg_val = gain;
In the similar way, put_unaligned and use gain[0], gain[1];
> + ret = regmap_write(st->regmap, AD485X_REG_CHX_GAIN_MSB(ch),
> + reg_val >> 8);
> + if (ret)
> + return ret;
> +
> + return regmap_write(st->regmap, AD485X_REG_CHX_GAIN_LSB(ch),
> + reg_val & 0xFF);
> +}
...
> +static int ad485x_get_calibbias(struct ad485x_state *st, int ch, int *val)
> +{
> + if (st->info->resolution == 16) {
> + *val = msb << 8;
> + *val |= mid;
> + *val = sign_extend32(*val, 15);
get_unaligned_be16()
> + } else {
> + *val = msb << 12;
> + *val |= mid << 4;
> + *val |= lsb >> 4;
get_unaligned_be24()
> + *val = sign_extend32(*val, 19);
> + }
> +}
...
> +static int ad485x_set_calibbias(struct ad485x_state *st, int ch, int val)
> +{
> + u8 buf[3] = { 0 };
0 is not needed.
> + int ret;
> +
> + if (val < 0)
> + return -EINVAL;
> +
> + if (st->info->resolution == 16)
> + put_unaligned_be16(val, buf);
> + else
> + put_unaligned_be24(val << 4, buf);
You see, you even did this! Why not in the above functions?
> +}
...
> +static const unsigned int ad485x_scale_avail[] = {
> + 2500, 5000, 10000, 6250, 12500, 20000, 25000, 40000, 50000, 80000,
It's better to have pow-of-2 of numbers on one line. So here each line up to 8?
2500, 5000, 10000, 6250, 12500, 20000, 25000, 40000, /* 0-7 */
50000, 80000, /* 8-9 */
> +};
> +
> +static void __ad485x_get_scale(struct ad485x_state *st, int scale_tbl,
> + unsigned int *val, unsigned int *val2)
> +{
> + const struct ad485x_chip_info *info = st->info;
> + const struct iio_chan_spec *chan = &info->channels[0];
> + unsigned int tmp;
> +
> + tmp = (scale_tbl * 1000000ULL) >> chan->scan_type.realbits;
MICRO? MEGA?
> + *val = tmp / 1000000;
> + *val2 = tmp % 1000000;
Ditto.
> +}
--
With Best Regards,
Andy Shevchenko
Powered by blists - more mailing lists