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

[Marcelo Schmitt <marcelo.schmitt1@xxxxxxxxx>]

On 06/26, Alexandru Ardelean wrote:
> On Wed, Jun 26, 2024 at 12:56 AM Marcelo Schmitt
> <marcelo.schmitt@xxxxxxxxxx> 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@xxxxxxxxxx>
> > ---
> >  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