Re: [PATCH v2 6/7] iio: adc: ad485x: add ad485x driver
From: Andy Shevchenko
Date: Sat Oct 05 2024 - 14:54:44 EST
On Fri, Oct 4, 2024 at 5:12 PM Antoniu Miclaus
<antoniu.miclaus@xxxxxxxxxx> 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