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: <CA+GgBR9sKzBptH3dm0Eg6Gi7-369vvHerC6EQvzTC_qBe82LYQ@mail.gmail.com> Date: Fri, 6 Sep 2024 17:03:48 +0300 From: Alexandru Ardelean <aardelean@...libre.com> To: David Lechner <dlechner@...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 Fri, Sep 6, 2024 at 4:33 PM David Lechner <dlechner@...libre.com> wrote: > > 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!? Yep ¯\_(ツ)_/¯ > > > >> > >>> + 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). ack; will remove it then from ad7606_read_samples()
Powered by blists - more mailing lists