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

[Alexandru Ardelean]

On Fri, Sep 6, 2024 at 4:33 PM David Lechner <dlechner@xxxxxxxxxxxx> wrote:
>
> On 9/6/24 12:34 AM, Alexandru Ardelean wrote:
> > On Fri, Sep 6, 2024 at 2:30 AM David Lechner <dlechner@xxxxxxxxxxxx> 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()