linux-kernel - Re: [PATCH v4 8/8] iio: adc: ad7606: add support for AD7606C-{16,18} parts

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

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <52eb0187-056b-413a-a64a-6fd001c27132@baylibre.com>
Date: Fri, 6 Sep 2024 08:33:21 -0500
From: David Lechner <dlechner@...libre.com>
To: Alexandru Ardelean <aardelean@...libre.com>
Cc: linux-iio@...r.kernel.org, linux-kernel@...r.kernel.org,
 devicetree@...r.kernel.org, jic23@...nel.org, krzk+dt@...nel.org,
 robh@...nel.org, lars@...afoo.de, michael.hennerich@...log.com,
 gstols@...libre.com
Subject: Re: [PATCH v4 8/8] iio: adc: ad7606: add support for AD7606C-{16,18}
 parts

On 9/6/24 12:34 AM, Alexandru Ardelean wrote:
> On Fri, Sep 6, 2024 at 2:30 AM David Lechner <dlechner@...libre.com> wrote:
>>
>> On 9/5/24 3:24 AM, Alexandru Ardelean wrote:


>>> -static int ad7606_read_samples(struct ad7606_state *st)
>>> +static int ad7606_read_samples(struct ad7606_state *st, bool sign_extend_samples)
>>>  {
>>> +     unsigned int storagebits = st->chip_info->channels[1].scan_type.storagebits;
>>
>> Why [1]? Sure, they are all the same, but [0] would seem less arbitrary.
> 
> [0] is the timestamp channel.

Oh, that's weird. First channel but last scan index!?


>>
>>> +     if (ret)
>>> +             return ret;
>>> +
>>> +     if (storagebits == 16 || !sign_extend_samples)
>>> +             return 0;
>>> +
>>> +     /* For 18 bit samples, we need to sign-extend samples to 32 bits */
>>> +     for (i = 0; i < num; i++)
>>> +             data32[i] = sign_extend32(data32[i], 17);> +
>>> +     return 0;
>>>  }
>>>
>>>  static irqreturn_t ad7606_trigger_handler(int irq, void *p)
>>> @@ -124,11 +176,11 @@ static irqreturn_t ad7606_trigger_handler(int irq, void *p)
>>>
>>>       guard(mutex)(&st->lock);
>>>
>>> -     ret = ad7606_read_samples(st);
>>> +     ret = ad7606_read_samples(st, true);
>>
>> Shouldn't the sign_extend parameter depend on if the data is unipolar or bipolar?
> 
> [c1]
> Sign-extension is only needed for 18-bit samples.
> 16-bit samples are already properly sign(ed), but to 16-bits.
> 
> It's a slight performance improvement, that may look quirky here.
> The idea here, is that for ad7606_scan_direct() we only need to
> sign-extend 1 sample of the 8 samples we get.
> And we need to sign-extend it to 32 bits regardless of it being 16-bit
> or 18-bit.
> 
> In ad7606_trigger_handler(), the 16-bit samples were pushed as-is.
> Which means that we need to sign-extend the samples at least for
> 18-bits (as it is a new part)
> The question now becomes if we should sign-extend to 32-bits, 16-bit
> samples in ad7606_trigger_handler(), as that may break some ABI.
> 

Sign extension should not be needed at all for buffered reads (that is
what scan_type is for). So sign extension should only be needed for
the direct read when returning a raw value via sysfs (raw read).