| 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: <1f2b8d91-19be-46b7-9202-824aa177dff6@baylibre.com>
Date: Thu, 7 Nov 2024 10:13:34 -0600
From: David Lechner <dlechner@...libre.com>
To: "Miclaus, Antoniu" <Antoniu.Miclaus@...log.com>,
"jic23@...nel.org" <jic23@...nel.org>,
"conor+dt@...nel.org" <conor+dt@...nel.org>,
"linux-iio@...r.kernel.org" <linux-iio@...r.kernel.org>,
"devicetree@...r.kernel.org" <devicetree@...r.kernel.org>,
"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
"linux-pwm@...r.kernel.org" <linux-pwm@...r.kernel.org>
Subject: Re: [PATCH v5 6/6] iio: adc: ad4851: add ad485x driver
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).
>>> +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.
>>> +
>>> +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),
>>
>>
Powered by blists - more mailing lists