Received-SPF: pass (google.com: domain of linux-kernel+bounces-137337-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.199.223 as permitted sender) client-ip=147.75.199.223;
Precedence: bulk
MIME-Version: 1.0
References: <cover.1712585500.git.marcelo.schmitt@analog.com>
 <1d95d7d023dad69b894a2d0e7b0bad9d569ae382.1712585500.git.marcelo.schmitt@analog.com>
 <CAMknhBEMDg3YF5pvoKJ-6y0Y5OJpmBthWfogCjy90B=F84SvzA@mail.gmail.com> <ZhVoTi2amNTOJ4eS@debian-BULLSEYE-live-builder-AMD64>
In-Reply-To: <ZhVoTi2amNTOJ4eS@debian-BULLSEYE-live-builder-AMD64>
From: David Lechner <dlechner@baylibre.com>
Date: Tue, 9 Apr 2024 11:44:26 -0500
Message-ID: <CAMknhBGVkv9sP0pjmmdFngKmGVu+G4Y3MUDG199az1wOUtwasw@mail.gmail.com>
Subject: Re: [PATCH v2 2/2] iio: adc: Add support for AD4000
To: Marcelo Schmitt <marcelo.schmitt1@gmail.com>
Cc: Marcelo Schmitt <marcelo.schmitt@analog.com>, lars@metafoo.de, 
	Michael.Hennerich@analog.com, jic23@kernel.org, robh+dt@kernel.org, 
	krzysztof.kozlowski+dt@linaro.org, conor+dt@kernel.org, 
	linux-iio@vger.kernel.org, devicetree@vger.kernel.org, 
	linux-kernel@vger.kernel.org
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable

On Tue, Apr 9, 2024 at 11:09=E2=80=AFAM Marcelo Schmitt
<marcelo.schmitt1@gmail.com> wrote:
>
> On 04/08, David Lechner wrote:
> > On Mon, Apr 8, 2024 at 9:32=E2=80=AFAM Marcelo Schmitt
> > <marcelo.schmitt@analog.com> wrote:
> > >

..

> >
> > I also still have doubts about using IIO_BE and 8-bit xfers when it
> > comes to adding support later to achieve max sample rate with a SPI
> > offload. For example to get 2MSPS with an 18-bit chip, it will require
> > an approx 33% faster SPI clock than the actual slowest clock possible
> > because it will have to read 6 extra bits per sample. I didn't check
> > the specs, but this may not even be physically possible without
> > exceeding the datasheet max SPI clock rate. Also errors could be
> > reduced if we could actually use the slowest allowable SPI clock rate.
> > Furthermore, the offload hardware would have to be capable of adding
> > an extra byte per sample for 18 and 20-bit chips when piping the data
> > to DMA in order to get the 32-bit alignment in the buffer required by
> > IIO scan_type and the natural alignment requirements of IIO buffers in
> > general.
>
> Maybe I should already implement support for reading with SPI offload
> rather than doing it after this set is merged?
> So we can test what happens at faster sample rates before we commit to a =
solution.
>

Yes, that sounds like a wise thing to do.

>
> >
> > > +               } data;
> > > +               s64 timestamp __aligned(8);
> > > +       } scan;
> > > +       __be16 tx_buf __aligned(IIO_DMA_MINALIGN);
> > > +       __be16 rx_buf;
> > > +};
> >
> > scan.data is used as SPI rx_buf so __aligned(IIO_DMA_MINALIGN); needs
> > to be moved to the scan field.
>
> I have already tried it. Maybe I did something wrong besides buffer align=
ment
> at that time. Will give it another try.

This is the alignment for DMA cache coherency. So it should not have
any affect on the actual data read, only performance.


> > > +static void ad4000_config(struct ad4000_state *st)
> > > +{
> > > +       unsigned int reg_val;
> > > +       int ret;
> > > +
> > > +       reg_val =3D FIELD_PREP(AD4000_TURBO, 1);
> >
> > Since the driver in it's current state can get anywhere near the max
> > sample rate of ~1MSPS, I don't think it makes sense to enable turbo at
> > this point.
> >
>
> This is just enabling turbo at start up. If not enabling turbo during pro=
be,
> we would want(need?) to provide some interface for that, which might not =
be
> much desired.
>

TURBO is only needed to achieve the max sample rate of 500k/1M/2MSPS
on the various chips by skipping powering down some circuitry between
samples. We can't get anywhere close to that in Linux without some
sort of SPI offloading. So, for now, we might as well leave it
disabled and save some power.


> > > +
> > > +       st->pin_gain =3D AD4000_1_GAIN;
> > > +       if (device_property_present(&spi->dev, "adi,gain-milli")) {
> > > +               u32 val;
> >
> > Should it be an error if adi,gain-milli is set on non-adaq chips?
>
> Maybe. We should not change the scale if it's a chip that don't have the
> amplifier in front of the ADC. I think the best handling would be to just
> ignore adi,gain-milli if it's not an ADAQ device. Maybe better add a DT
> constraint,
>   - if:
>       properties:
>         compatible:
>           contains:
>             enum:
>               - adi,adaq4001
>               - adi,adaq4003
>     then:
>       properties:
>         adi,gain-milli: false
> ?

I think this is missing a not:, but otherwise yes this should be in
the DT bindings.

Even with that though, I would still be helpful to readers of the
driver to at least have a comment here pointing out that this property
and related gain scaling only applies to ADAQ chips.