On Mon, Sep 23, 2024 at 12:17 PM Antoniu Miclaus
<antoniu.miclaus@xxxxxxxxxx> wrote:
>
> Add support for the AD485X DAS familiy.
>
> Signed-off-by: Antoniu Miclaus <antoniu.miclaus@xxxxxxxxxx>
> ---
...
> +static int ad485x_find_opt(bool *field, u32 size, u32 *ret_start)
> +{
> + int i, cnt = 0, max_cnt = 0, start, max_start = 0;
> +
> + 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 -EIO;
EIO seems an odd choice since this function doesn't actually do any
I/O. Maybe EINVAL would be better?
> +
> + *ret_start = max_start;
> +
> + return max_cnt;
> +}
> +
...
> +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;
> +
Need to check both val and val2 for negative here before converting to unsigned.
if (val < 0 || val2 < 0)
return -EINVAL;
> + gain = val * 1000000 + val2;
> + gain = gain * 32768;
> + gain = DIV_U64_ROUND_CLOSEST(gain, 1000000);
> +
> + reg_val = gain;
> +
> + guard(mutex)(&st->lock);
> +
> + 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,
> + int *val2)
> +{
val2 is unused and can be removed
> + unsigned int lsb, mid, msb;
> + int ret;
> +
> + guard(mutex)(&st->lock);
> +
> + ret = regmap_read(st->regmap, AD485X_REG_CHX_OFFSET_MSB(ch),
> + &msb);
> + if (ret)
> + return ret;
> +
> + ret = regmap_read(st->regmap, AD485X_REG_CHX_OFFSET_MID(ch),
> + &mid);
> + if (ret)
> + return ret;
> +
> + ret = regmap_read(st->regmap, AD485X_REG_CHX_OFFSET_LSB(ch),
> + &lsb);
> + if (ret)
> + return ret;
> +
> + if (st->info->resolution == 16) {
> + *val = msb << 8;
> + *val |= mid;
> + *val = sign_extend32(*val, 15);
> + } else {
> + *val = msb << 12;
> + *val |= mid << 4;
> + *val |= lsb >> 4;
> + *val = sign_extend32(*val, 19);
> + }
> +
> + return IIO_VAL_INT;
> +}
> +
> +static int ad485x_set_calibbias(struct ad485x_state *st, int ch, int val,
> + int val2)
> +{
val2 is unused here. It would also be a good idea to implement the
write_raw_get_fmt callback to select IIO_VAL_INT for this attribute to
avoid having to deal with negative val2.
> + unsigned int lsb, mid, msb;
> + int ret;
Should check for negative val here before converting to unsigned.
> +
> + if (st->info->resolution == 16) {
> + lsb = 0;
> + mid = val & 0xFF;
> + msb = (val >> 8) & 0xFF;
> + } else {
> + lsb = (val << 4) & 0xFF;
> + mid = (val >> 4) & 0xFF;
> + msb = (val >> 12) & 0xFF;
> + }
> +
> + guard(mutex)(&st->lock);
> +
> + ret = regmap_write(st->regmap, AD485X_REG_CHX_OFFSET_LSB(ch), lsb);
> + if (ret)
> + return ret;
> +
> + ret = regmap_write(st->regmap, AD485X_REG_CHX_OFFSET_MID(ch), mid);
> + if (ret)
> + return ret;
> +
> + return regmap_write(st->regmap, AD485X_REG_CHX_OFFSET_MSB(ch), msb);
> +}
> +
...
> +static int ad485x_set_offset(struct ad485x_state *st,
> + const struct iio_chan_spec *chan, int val)
> +{
> + guard(mutex)(&st->lock);
> +
> + if (st->offsets[chan->channel] != val) {
> + st->offsets[chan->channel] = val;
> + /* Restore to the default range if offset changes */
> + if (st->offsets[chan->channel])
> + return regmap_write(st->regmap,
> + AD485X_REG_CHX_SOFTSPAN(chan->channel),
> + AD485X_SOFTSPAN_N40V_40V);
> + return regmap_write(st->regmap,
> + AD485X_REG_CHX_SOFTSPAN(chan->channel),
> + AD485X_SOFTSPAN_0V_40V);
> + }
> +
> + return 0;
> +}
I'm not sure I understand the relationship between softspan and the
offset. A raw value of 0 always means we measured 0V no matter what
the softspan setting is. So it seems like the offset should always be
0.
I'm guessing the intent was to handle bipolar vs. unipolar softspans,
but this doesn't actually work mathematically.
So far, I've only seen inputs that can be bipolar or unipolar
specified in the devicetree. I'm not aware of a way to select this at
runtime.
> +static struct iio_chan_spec_ext_info ad4858_ext_info[] = {
> + IIO_ENUM("packet_format", IIO_SHARED_BY_ALL, &ad4858_packet_fmt),
> + IIO_ENUM_AVAILABLE("packet_format",
> + IIO_SHARED_BY_ALL, &ad4858_packet_fmt),
> + {},
> +};
> +
> +static struct iio_chan_spec_ext_info ad4857_ext_info[] = {
> + IIO_ENUM("packet_format", IIO_SHARED_BY_ALL, &ad4857_packet_fmt),
> + IIO_ENUM_AVAILABLE("packet_format",
> + IIO_SHARED_BY_ALL, &ad4857_packet_fmt),
> + {},
> +};
Usually, something like this packet format would be automatically
selected when buffered reads are enabled based on what other features
it provides are needed, i.e only enable the status bits when events
are enabled.
(For those that didn't read the datasheet, the different packet
formats basically enable extra status bits per sample. And in the case
of oversampling, one of the formats also selects a reduced number of
sample bits.)
We have quite a few parts in the pipline right like this one that have
per-sample status bits. In the past, these were generally handled with
IIO events, but this doesn't really work for these high-speed backends
since the data is being piped directly to DMA and we don't look at
each sample in the ADC driver. So it would be worthwhile to try to
find some general solution here for handling this sort of thing.
> +
> +static const struct iio_chan_spec ad4858_channels[] = {
> + AD485X_IIO_CHANNEL(0, 20, 32, ad4858_ext_info),
> + AD485X_IIO_CHANNEL(1, 20, 32, ad4858_ext_info),
> + AD485X_IIO_CHANNEL(2, 20, 32, ad4858_ext_info),
> + AD485X_IIO_CHANNEL(3, 20, 32, ad4858_ext_info),
> + AD485X_IIO_CHANNEL(4, 20, 32, ad4858_ext_info),
> + AD485X_IIO_CHANNEL(5, 20, 32, ad4858_ext_info),
> + AD485X_IIO_CHANNEL(6, 20, 32, ad4858_ext_info),
> + AD485X_IIO_CHANNEL(7, 20, 32, ad4858_ext_info),
> +};
> +
> +static const struct iio_chan_spec ad4857_channels[] = {
> + AD485X_IIO_CHANNEL(0, 16, 16, ad4857_ext_info),
> + AD485X_IIO_CHANNEL(1, 16, 16, ad4857_ext_info),
> + AD485X_IIO_CHANNEL(2, 16, 16, ad4857_ext_info),
> + AD485X_IIO_CHANNEL(3, 16, 16, ad4857_ext_info),
> + AD485X_IIO_CHANNEL(4, 16, 16, ad4857_ext_info),
> + AD485X_IIO_CHANNEL(5, 16, 16, ad4857_ext_info),
> + AD485X_IIO_CHANNEL(6, 16, 16, ad4857_ext_info),
> + AD485X_IIO_CHANNEL(7, 16, 16, ad4857_ext_info),
> +};
How does 16-bit storage size work when packet format is 24-bit?
