--
Antoniu Miclăuş
> -----Original Message-----
> From: David Lechner <dlechner@xxxxxxxxxxxx>
> Sent: Thursday, November 7, 2024 6:14 PM
> To: Miclaus, Antoniu <Antoniu.Miclaus@xxxxxxxxxx>; jic23@xxxxxxxxxx;
> conor+dt@xxxxxxxxxx; linux-iio@xxxxxxxxxxxxxxx; devicetree@xxxxxxxxxxxxxxx;
> linux-kernel@xxxxxxxxxxxxxxx; linux-pwm@xxxxxxxxxxxxxxx
> Subject: Re: [PATCH v5 6/6] iio: adc: ad4851: add ad485x driver
>
> [External]
>
> On 11/7/24 4:51 AM, Miclaus, Antoniu wrote:
> >>> + if (osr == 1) {
> >>> + ret = regmap_update_bits(st->regmap, AD4851_REG_PACKET,
> >>> + AD4851_PACKET_FORMAT_MASK,
> >> 0);
> >>
> >> regmap_clear_bits()
> >>
> >>> + if (ret)
> >>> + return ret;
> >>> +
> >>> + st->resolution_boost_enabled = false;
> >>> + } else {
> >>> + ret = regmap_update_bits(st->regmap, AD4851_REG_PACKET,
> >>> + AD4851_PACKET_FORMAT_MASK,
> >> 1);
> >>
> >> regmap_set_bits()
> > Packet format is 2 bits wide. Not sure how can I write 1 if I use regmap
> set_bits
> > Should I do 2 separate masks?
>
> Sorry, I missed that detail. In that case, using FIELD_PREP() here would
> make that clear (even if it isn't technically required).
>
Sure I will do FIELD_PREP.
>
> >>> +static int ad4851_set_calibscale(struct ad4851_state *st, int ch, int val,
> >>> + int val2)
> >>> +{
> >>> + u64 gain;
> >>> + u8 buf[0];
> >>> + int ret;
> >>> +
> >>> + if (val < 0 || val2 < 0)
> >>> + return -EINVAL;
> >>> +
> >>> + gain = val * MICRO + val2;
> >>> + gain = DIV_U64_ROUND_CLOSEST(gain * 32768, MICRO);
> >>> +
> >>> + put_unaligned_be16(gain, buf);
> >>> +
> >>> + guard(mutex)(&st->lock);
> >>> +
> >>> + ret = regmap_write(st->regmap, AD4851_REG_CHX_GAIN_MSB(ch),
> >>> + buf[0]);
> >>> + if (ret)
> >>> + return ret;
> >>> +
> >>> + return regmap_write(st->regmap, AD4851_REG_CHX_GAIN_LSB(ch),
> >>> + buf[1]);
> >>> +}
> >>> +
> >>
> >> I'm pretty sure that calibscale and calibbias also need to take into
> >> account if resolution boost is enabled or not.
> >
> > Can you please detail a bit on this topic? I am not sure what I should do.
> >
>
> We haven't implemented oversampling yet in ad4695 yet, so I don't know
> exactly what we need to do either. ;-)
>
> But this is how I would test it to see if it is working correctly or
> not. We will need to test this with a 20-bit chip since that is the
> only one that will change the _scale attribute when oversampling is
> enabled.
>
> First, with oversampling disabled (_oversampling_ratio = 1), generate
> a constant voltage of 1V for the input. Read the _raw attribute. Let's
> call this value raw0. Read the _scale attribute, call it scale0 and
> the _offset attribute, call it offset0.
>
> Then we should have (raw0 + offset0) * scale0 = 1000 mV (+/- some
> noise).
>
> Then change the offset calibrate to 100 mV. To do this, we reverse
> the calculation 100 mV / scale0 = calibbias (raw units). Write the
> raw value to the _calibbias attribute. Then read the _raw
> attribute again, call it raw0_with_calibbias.
>
> This time, we should have (raw0_with_calibbias + offset0) * scale0
> = 1100 mV (+/- some noise).
>
> Then set _calibbias back to 0 and repeat the above by setting the
> _calibscale attribute to 0.90909 (this is 1 / 1.1, which should
> add 10% to the measured raw value). Read, the _raw attribute again,
> call it raw0_with_caliscale.
>
> This time, we should have (raw0_with_caliscale + offset0) * scale0
> = 1100 mV (+/- some noise).
>
> Set _calibscale back to 0. Then set _oversampling_ratio to 2. Read
> _scale and _offset again, call these scale1 and offset1.
>
> Then repeat the steps above using scale1 and offset1 in the
> calculations. The raw values will be different but the resulting
> processed values (mV) should all be the same if the attributes
> are implemented correctly.
>
> >>> +static const unsigned int ad4851_scale_table[][2] = {
> >>> + { 2500, 0x0 },
> >>> + { 5000, 0x1 },
> >>> + { 5000, 0x2 },
> >>> + { 10000, 0x3 },
> >>> + { 6250, 0x04 },
> >>> + { 12500, 0x5 },
> >>> + { 10000, 0x6 },
> >>> + { 20000, 0x7 },
> >>> + { 12500, 0x8 },
> >>> + { 25000, 0x9 },
> >>> + { 20000, 0xA },
> >>> + { 40000, 0xB },
> >>> + { 25000, 0xC },
> >>> + { 50000, 0xD },
> >>> + { 40000, 0xE },
> >>> + { 80000, 0xF },
> >>> +};
> >>
> >> I'm not sure how this table is supposed to work since there are
> >> multiple entries with the same voltage value. Probably better
> >> would be to just have the entries for the unipolar/unsigned ranges.
> >> Then if applying this to a differential/signed channel, just add
> >> 1 to resulting register value before writing it to the register.
> >> Or make two different tables, one for unsigned and one for signed
> >> channels.
> >
> > It is stated in the set_scale function comment how this table works.
> > This table contains range-register value pair.
> > Always the second value corresponds to the single ended mode.
> >>
>
> Yes, I understand that part. The problem is that values like 10000
> are listed twice in the table, so if we have a softspan of 0..+10V
> or -10V..+10V, how do we know which 10000 to use to get the right
> register value? This is why I think it needs to be 2 different
> tables.
Yes there are values listed twice, but there is a rule that is handled by
the set/get scale functions.
/*
* Adjust the softspan value (differential or single ended)
* based on the scale value selected channel type.
*
* If the channel is not differential then continue iterations
* until the next matching scale value which always corresponds
* to the single ended mode.
*/
I was already asked in previous patches to detail the comment in the
set_scale function so I did it.
> >>> +
> >>> +static const struct iio_chan_spec ad4858_channels[] = {
> >>> + AD4851_IIO_CHANNEL(0, 0, 20),
> >>> + AD4851_IIO_CHANNEL(1, 0, 20),
> >>> + AD4851_IIO_CHANNEL(2, 0, 20),
> >>> + AD4851_IIO_CHANNEL(3, 0, 20),
> >>> + AD4851_IIO_CHANNEL(4, 0, 20),
> >>> + AD4851_IIO_CHANNEL(5, 0, 20),
> >>> + AD4851_IIO_CHANNEL(6, 0, 20),
> >>> + AD4851_IIO_CHANNEL(7, 0, 20),
> >>> + AD4851_IIO_CHANNEL(0, 1, 20),
> >>> + AD4851_IIO_CHANNEL(1, 1, 20),
> >>> + AD4851_IIO_CHANNEL(2, 1, 20),
> >>> + AD4851_IIO_CHANNEL(3, 1, 20),
> >>> + AD4851_IIO_CHANNEL(4, 1, 20),
> >>> + AD4851_IIO_CHANNEL(5, 1, 20),
> >>> + AD4851_IIO_CHANNEL(6, 1, 20),
> >>> + AD4851_IIO_CHANNEL(7, 1, 20),
> >>> +};
> >>> +
> >>> +static const struct iio_chan_spec ad4857_channels[] = {
> >>> + AD4851_IIO_CHANNEL(0, 0, 16),
> >>> + AD4851_IIO_CHANNEL(1, 0, 16),
> >>> + AD4851_IIO_CHANNEL(2, 0, 16),
> >>> + AD4851_IIO_CHANNEL(3, 0, 16),
> >>> + AD4851_IIO_CHANNEL(4, 0, 16),
> >>> + AD4851_IIO_CHANNEL(5, 0, 16),
> >>> + AD4851_IIO_CHANNEL(6, 0, 16),
> >>> + AD4851_IIO_CHANNEL(7, 0, 16),
> >>> + AD4851_IIO_CHANNEL(0, 1, 16),
> >>> + AD4851_IIO_CHANNEL(1, 1, 16),
> >>> + AD4851_IIO_CHANNEL(2, 1, 16),
> >>> + AD4851_IIO_CHANNEL(3, 1, 16),
> >>> + AD4851_IIO_CHANNEL(4, 1, 16),
> >>> + AD4851_IIO_CHANNEL(5, 1, 16),
> >>> + AD4851_IIO_CHANNEL(6, 1, 16),
> >>> + AD4851_IIO_CHANNEL(7, 1, 16),
> >>> +};
> >>
> >> I don't think it is valid for two channels to have the same scan_index.
> >> And since this is simultaneous sampling and we don't have control over
> >> the order in which the data is received from the backend, to get the
> >> ordering correct, we will likely have to make this:
> >>
> > I am not sure which of these channels have the same index.
> > scan_index is index + diff * 8 in the channel definition.
> >
>
> scan_index indicates the order in which a data value for a channel
> will appear in the buffer when doing a buffered read. So all scan_index
> for any channel 0 need to be less than all scan_index for all
> channel 1, and so on.
>
> So in the suggestion quoted below, the scan_index parameter
> just gets assigned directly to .scan_index without any
> additional calculations.
>
> >> #define AD4851_IIO_CHANNEL(scan_index, channel, diff, bits) \
> >> ...
> >>
> >> AD4851_IIO_CHANNEL(0, 0, 0, 16),
> >> AD4851_IIO_CHANNEL(1, 0, 1, 16),
> >> AD4851_IIO_CHANNEL(2, 1, 0, 16),
> >> AD4851_IIO_CHANNEL(3, 1, 1, 16),
> >> AD4851_IIO_CHANNEL(4, 2, 0, 16),
> >> AD4851_IIO_CHANNEL(5, 2, 1, 16),
> >> AD4851_IIO_CHANNEL(6, 3, 0, 16),
> >> AD4851_IIO_CHANNEL(7, 3, 1, 16),
> >> AD4851_IIO_CHANNEL(8, 4, 0, 16),
> >> AD4851_IIO_CHANNEL(9, 4, 1, 16),
> >> AD4851_IIO_CHANNEL(10, 5, 0, 16),
> >> AD4851_IIO_CHANNEL(11, 5, 1, 16),
> >> AD4851_IIO_CHANNEL(12, 6, 0, 16),
> >> AD4851_IIO_CHANNEL(13, 6, 1, 16),
> >> AD4851_IIO_CHANNEL(14, 7, 0, 16),
> >> AD4851_IIO_CHANNEL(15, 7, 1, 16),
> >>
> >>
