[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <ZnwW4Pf1OS8KjkVp@debian-BULLSEYE-live-builder-AMD64>
Date: Wed, 26 Jun 2024 10:25:52 -0300
From: Marcelo Schmitt <marcelo.schmitt1@...il.com>
To: Alexandru Ardelean <aardelean@...libre.com>
Cc: Marcelo Schmitt <marcelo.schmitt@...log.com>, broonie@...nel.org,
lars@...afoo.de, Michael.Hennerich@...log.com, jic23@...nel.org,
robh+dt@...nel.org, krzysztof.kozlowski+dt@...aro.org,
conor+dt@...nel.org, nuno.sa@...log.com, dlechner@...libre.com,
corbet@....net, linux-iio@...r.kernel.org,
devicetree@...r.kernel.org, linux-spi@...r.kernel.org,
linux-doc@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v5 6/7] iio: adc: Add support for AD4000
On 06/26, Alexandru Ardelean wrote:
> On Wed, Jun 26, 2024 at 12:56 AM Marcelo Schmitt
> <marcelo.schmitt@...log.com> wrote:
> >
> > Add support for AD4000 series of low noise, low power, high speed,
> > successive approximation register (SAR) ADCs.
> >
>
> Hello :)
Hey Alexandru, nice to hear from you.
>
> Looks good overall.
> Just a few comments.
> The only one where I am not sure is about the enum-to-string mapping.
> If that's fine, we can leave this unchanged (from my side).
>
> > Signed-off-by: Marcelo Schmitt <marcelo.schmitt@...log.com>
> > ---
> > MAINTAINERS | 1 +
> > drivers/iio/adc/Kconfig | 12 +
> > drivers/iio/adc/Makefile | 1 +
> > drivers/iio/adc/ad4000.c | 711 +++++++++++++++++++++++++++++++++++++++
> > 4 files changed, 725 insertions(+)
> > create mode 100644 drivers/iio/adc/ad4000.c
> >
...
> > +enum ad4000_sdi {
> > + /* datasheet calls this "4-wire mode" (controller CS goes to ADC SDI!) */
> > + AD4000_SDI_MOSI,
> > + /* datasheet calls this "3-wire mode" (not related to SPI_3WIRE!) */
> > + AD4000_SDI_VIO,
> > + AD4000_SDI_CS,
> > +};
> > +
> > +/* maps adi,sdi-pin property value to enum */
> > +static const char * const ad4000_sdi_pin[] = {
> > + [AD4000_SDI_MOSI] = "",
>
> Maybe I missed a previous comment.
> And I'm also a little fuzzy on the details here, but in the DT this
> property has "high", "low", "cs".
> Is "low" the default if unspecified?
> Or should this string be "low"?
The default is to have MOSI connected to ADC SDI pin which was empty adi,sdi-pin
dt property in v5.
Will make the defalut explicit as "sdi" as suggested in dt-binding review.
>
> > + [AD4000_SDI_VIO] = "high",
> > + [AD4000_SDI_CS] = "cs",
> > +};
> > +
...
> > +
> > + st->gain_milli = 1000;
> > + if (chip->has_hardware_gain) {
> > + if (device_property_present(dev, "adi,gain-milli")) {
>
> Only if there is another version, it may be neat to reduce indentation
> here (a bit).
> Something like:
> if (chip->has_hardware_gain &&
> device_property_present(dev, "adi,gain-milli")) {
>
> }
>
looks good, will do.
Thanks
Powered by blists - more mailing lists