Re: [PATCH 2/6] iio: adc: ad4030: add driver for ad4030-24

Nuno Sá
2024-09-13T10:22:46Z

Hi Esteban,

Just one remark...

On Thu, 2024-08-22 at 14:39 -0500, David Lechner wrote:
> On 8/22/24 7:45 AM, Esteban Blanc wrote:
> > This adds a new driver for the Analog Devices INC. AD4030-24 ADC.
> >
> > The driver implements basic support for the AD4030-24 1 channel
> > differential ADC with hardware gain and offset control.
> >
> > Signed-off-by: Esteban Blanc <eblanc@baylibre.com>
> > ---
> >  MAINTAINERS              |   1 +
> >  drivers/iio/adc/Kconfig  |  13 +
> >  drivers/iio/adc/Makefile |   1 +
> >  drivers/iio/adc/ad4030.c | 854 +++++++++++++++++++++++++++++++++++++++++++++++
> >  4 files changed, 869 insertions(+)
> >
> > diff --git a/MAINTAINERS b/MAINTAINERS
> > index f17c42bea19c..6a5a0e7b7a51 100644
> > --- a/MAINTAINERS
> > +++ b/MAINTAINERS
> > @@ -420,6 +420,7 @@ R: Esteban Blanc <eblanc@baylibre.com>
> >  S: Supported
> >  W: https://ez.analog.com/linux-software-drivers
> >  F: Documentation/devicetree/bindings/iio/adc/adi,ad4030.yaml
> > +F: drivers/iio/adc/ad4030.c
> >
> >  AD5110 ANALOG DEVICES DIGITAL POTENTIOMETERS DRIVER
> >  M: Mugilraj Dhavachelvan <dmugil2000@gmail.com>
> > diff --git a/drivers/iio/adc/Kconfig b/drivers/iio/adc/Kconfig
> > index 88e8ce2e78b3..f4bd05780f6f 100644
> > --- a/drivers/iio/adc/Kconfig
> > +++ b/drivers/iio/adc/Kconfig
> > @@ -33,6 +33,19 @@ config AD4000
> >     To compile this driver as a module, choose M here: the module will be
> >     called ad4000.
> >
> > +config AD4030
> > + tristate "Analog Device AD4630 ADC Driver"
> > + depends on SPI
> > + depends on GPIOLIB
> > + select REGMAP_SPI
>
> It looks like we are just using REGMAP, not REGMAP_SPI.
>
> > + select IIO_BUFFER
>
> And also select IIO_TRIGGERED_BUFFER?
>
> > + help
> > +   Say yes here to build support for Analog Devices AD4030 and AD4630
> > high speed
> > +   SPI analog to digital converters (ADC).
> > +
> > +   To compile this driver as a module, choose M here: the module will be
> > +   called ad4030.
> > +
> >  config AD4130
> >   tristate "Analog Device AD4130 ADC Driver"
> >   depends on SPI
> > diff --git a/drivers/iio/adc/Makefile b/drivers/iio/adc/Makefile
> > index 8b80664c6d6b..0e4f833abf0b 100644
> > --- a/drivers/iio/adc/Makefile
> > +++ b/drivers/iio/adc/Makefile
> > @@ -7,6 +7,7 @@
> >  obj-$(CONFIG_AB8500_GPADC) += ab8500-gpadc.o
> >  obj-$(CONFIG_AD_SIGMA_DELTA) += ad_sigma_delta.o
> >  obj-$(CONFIG_AD4000) += ad4000.o
> > +obj-$(CONFIG_AD4030) += ad4030.o
> >  obj-$(CONFIG_AD4130) += ad4130.o
> >  obj-$(CONFIG_AD4695) += ad4695.o
> >  obj-$(CONFIG_AD7091R) += ad7091r-base.o
> > diff --git a/drivers/iio/adc/ad4030.c b/drivers/iio/adc/ad4030.c
> > new file mode 100644
> > index 000000000000..a981dce988e5
> > --- /dev/null
> > +++ b/drivers/iio/adc/ad4030.c
> > @@ -0,0 +1,854 @@
> > +// SPDX-License-Identifier: GPL-2.0-only
> > +/*
> > + * Analog Devices AD4030 and AD4630 ADC family driver.
> > + *
> > + * Copyright 2024 Analog Devices, Inc.
> > + * Copyright 2024 BayLibre, SAS
> > + *
> > + * based on code from:
> > + * Analog Devices, Inc.
> > + *   Sergiu Cuciurean <sergiu.cuciurean@analog.com>
> > + *   Nuno Sa <nuno.sa@analog.com>
> > + *   Marcelo Schmitt <marcelo.schmitt@analog.com>
> > + *   Liviu Adace <liviu.adace@analog.com>
> > + */
> > + .type = IIO_VOLTAGE, \
> > + .indexed = 1, \
> > + .channel = _idx * 2 + 2, \
> > + .scan_index = _idx * 2 + 1, \
> > + .extend_name = "Channel" #_idx " common byte part", \
>
> Labels are usually one word and reflect the datasheet name.
>
> Suggest `"common-mode" #_idx` or `"CM" #_idx` for this one.
>

Also, .extend_name is not to be used anymore... In the end of the day IIO will create
label files anyways but from what I remember about this, extend_name is not to be
directly used this anymore (so other think it's still fine). Instead, use the label
callback.

- Nuno Sá
>
>