On 11/4/21 10:24 AM, Gwendal Grignou wrote:
> On Wed, Nov 3, 2021 at 11:37 AM Jonathan Cameron <jic23@xxxxxxxxxx> wrote:
>>
>> On Mon, 1 Nov 2021 00:18:19 -0700
>> Gwendal Grignou <gwendal@xxxxxxxxxxxx> wrote:
>>
>>> When user space application read iio buffer though libiio, data is
>>> converted (see iio_channel_convert()) using the _type sysfs parameter.
>>> In particular, scan_type.shift and scan_type.realbits are used to shift
>>> and tell on how many bits signed elements are encoded on.
>>>
>>> When reading elements directly using the raw sysfs attributes, the same
>>> rules for shifting and signing should apply.
>>>
>>> Use channel definition as root of trust and replace constant with
>>> them for the simple cases.
>>>
>>> Signed-off-by: Gwendal Grignou <gwendal@xxxxxxxxxxxx>
>>
>> Hmm. I'd have been tempted to do this as a long series, at least
>> partly so it would let me pick up the ones I'm happy with + also
>> we may create some history that needs backporting at some stage
>> and that's a mess if we have code touching lots of drivers in one patch.
> I will split this patch, one per driver so that you can pick only the
> safe changes only.
> There are many more drivers that would benefit from using scan_type,
> but they require more restructuring.
>
>>
>> Ludovic, Eugen, This change raised a question about the current
>> code in at91-sama5d2_adc.c
>>
>>> ---
>>> drivers/iio/accel/bma220_spi.c | 3 ++-
>>> drivers/iio/accel/kxcjk-1013.c | 3 ++-
>>> drivers/iio/accel/mma7455_core.c | 3 ++-
>>> drivers/iio/accel/sca3000.c | 5 +++--
>>> drivers/iio/accel/stk8312.c | 2 +-
>>> drivers/iio/accel/stk8ba50.c | 3 ++-
>>> drivers/iio/adc/ad7266.c | 3 ++-
>>> drivers/iio/adc/at91-sama5d2_adc.c | 3 ++-
>>> drivers/iio/adc/ti-adc12138.c | 3 ++-
>>> drivers/iio/magnetometer/mag3110.c | 6 ++++--
>>> 10 files changed, 22 insertions(+), 12 deletions(-)
>>>
>>> diff --git a/drivers/iio/accel/bma220_spi.c b/drivers/iio/accel/bma220_spi.c
>>> index bc4c626e454d3..812d6749b24a7 100644
>>> --- a/drivers/iio/accel/bma220_spi.c
>>> +++ b/drivers/iio/accel/bma220_spi.c
>>> @@ -125,7 +125,8 @@ static int bma220_read_raw(struct iio_dev *indio_dev,
>>> ret = bma220_read_reg(data->spi_device, chan->address);
>>> if (ret < 0)
>>> return -EINVAL;
>>> - *val = sign_extend32(ret >> BMA220_DATA_SHIFT, 5);
>>> + *val = sign_extend32(ret >> chan->scan_type.shift,
>>> + chan->scan_type.realbits - 1);
>>
>> The BMA220_DATA_SHIFT define is now only used as the value for .shift, so
>> could you move the value inline to there and get rid of the define.
>>
>> That will be match what is done for all the other parts of scan_type.
> Done.
>>
>>> return IIO_VAL_INT;
>>> case IIO_CHAN_INFO_SCALE:
>>> ret = bma220_read_reg(data->spi_device, BMA220_REG_RANGE);
>>
>>
>>> diff --git a/drivers/iio/accel/sca3000.c b/drivers/iio/accel/sca3000.c
>>> index c6b75308148aa..938eb6bda73b3 100644
>>> --- a/drivers/iio/accel/sca3000.c
>>> +++ b/drivers/iio/accel/sca3000.c
>>> @@ -730,8 +730,9 @@ static int sca3000_read_raw(struct iio_dev *indio_dev,
>>> mutex_unlock(&st->lock);
>>> return ret;
>>> }
>>> - *val = (be16_to_cpup((__be16 *)st->rx) >> 3) & 0x1FFF;
>>> - *val = sign_extend32(*val, 12);
>>> + *val = (be16_to_cpup((__be16 *)st->rx) >>
>>> + chan->scan_type.shift) & 0x1FFF;
>>
>> While here, GENMASK(12, 0) for the mask might be a nice to have..
> Actually, it may not be needed at all: we push a 16bit field 3 bits to
> the left, so 13 bits remain anyway.
>>
>>> + *val = sign_extend32(*val, chan->scan_type.realbits - 1);
>>> } else {
>>> /* get the temperature when available */
>>> ret = sca3000_read_data_short(st,
>> The code following this is exciting as well... and would benefit from
>> being rewritten as be16_to_cpup() with a shift and mask but that's a job for
>> a different patch, or you could add the scan_type to the temperature channel and
>> add it to this series using that... It's unsigned unlike the above, so
>> it probably doesn't make sense to have a single path.
> Done
>>
>>
>>
>>
>>> diff --git a/drivers/iio/adc/at91-sama5d2_adc.c b/drivers/iio/adc/at91-sama5d2_adc.c
>>> index 4c922ef634f8e..92a57cf10fba4 100644
>>> --- a/drivers/iio/adc/at91-sama5d2_adc.c
>>> +++ b/drivers/iio/adc/at91-sama5d2_adc.c
>>> @@ -1586,7 +1586,8 @@ static int at91_adc_read_info_raw(struct iio_dev *indio_dev,
>>> *val = st->conversion_value;
>>> ret = at91_adc_adjust_val_osr(st, val);
>>> if (chan->scan_type.sign == 's')
>>> - *val = sign_extend32(*val, 11);
>>> + *val = sign_extend32(*val,
>>> + chan->scan_type.realbits - 1);
>>> st->conversion_done = false;
>> Hmm. This is exciting. I'm not sure if the current code is correct.
>> There is a comment that says it's a voltage channel if we are in this path
>> a few lines earlier, yet the only signed voltage channel is from the
>> macro AT91_SAMA5D2_CHAN_DIFF() which sets realbits = 14, not 12.
> From https://ww1.microchip.com/downloads/en/DeviceDoc/DS60001476B.pdf,
> the ADC is natively 12 bits, but can be configured to oversample to
> reach 14 bits resolution.
> |realbit| may actually be a function of
> IIO_CHAN_INFO_OVERSAMPLING_RATIO [aka "oversampling_ratio"] value.
>>
Hi,
Ever since the oversampling was implemented, the realbits was changed to 14.
This means that if oversampling is enabled, the data will have 14 bits
of precision.
If oversampling is disabled, it will have 12 bits of useful data, but it
will be shifted to the left by 2 bits, so in the end the result will be
14 bits, but with LSB 2 bits always zero. ( this is what
at91_adc_adjust_val_osr does )
So I think that maybe a fix for this commit should be done first :
6794e23fa3fe ("iio: adc: at91-sama5d2_adc: add support for oversampling
resolution")
which does not change the '11' value to '13'
and then you can apply your commit that starts using the 'realbits' field.
So in fact, another bit was used to obtain the sign.. I wonder if
someone uses these differential channels since I haven't heard of any
problem reported. And during my tests it looked to be fine, but maybe it
was a bit coincidence.
If I get my hands on a signal generator I will try to make a
differential GND to a sinus waveform and plot the results to see if it
works as expected
Nice catch and thanks,
Eugen
>>
>>> }
>>>
>>
>> The other changes all looked good to me.
> Thanks,
> Gwendal.
>>
>> Jonathan
>>
