linux-kernel - Re: [PATCH v5 6/7] iio: adc: Add support for AD4000

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]

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