Re: [PATCH v7 6/6] iio: adc: ad7192: Add AD7194 support

[David Lechner]

On Tue, Apr 30, 2024 at 11:30 AM Alisa-Dariana Roman
<alisadariana@xxxxxxxxx> wrote:
>
> Unlike the other AD719Xs, AD7194 has configurable channels. The user can
> dynamically configure them in the devicetree.
>
> Also modify config AD7192 description for better scaling.
>
> Signed-off-by: Alisa-Dariana Roman <alisa.roman@xxxxxxxxxx>
> ---
>  drivers/iio/adc/Kconfig  |  11 +++-
>  drivers/iio/adc/ad7192.c | 129 +++++++++++++++++++++++++++++++++++++--
>  2 files changed, 133 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/iio/adc/Kconfig b/drivers/iio/adc/Kconfig
> index 8db68b80b391..74fecc284f1a 100644
> --- a/drivers/iio/adc/Kconfig
> +++ b/drivers/iio/adc/Kconfig
> @@ -88,12 +88,17 @@ config AD7173
>           called ad7173.
>
>  config AD7192
> -       tristate "Analog Devices AD7190 AD7192 AD7193 AD7195 ADC driver"
> +       tristate "Analog Devices AD7192 and similar ADC driver"
>         depends on SPI
>         select AD_SIGMA_DELTA
>         help
> -         Say yes here to build support for Analog Devices AD7190,
> -         AD7192, AD7193 or AD7195 SPI analog to digital converters (ADC).
> +         Say yes here to build support for Analog Devices SPI analog to digital
> +         converters (ADC):
> +         - AD7190
> +         - AD7192
> +         - AD7193
> +         - AD7194
> +         - AD7195
>           If unsure, say N (but it's safe to say "Y").
>
>           To compile this driver as a module, choose M here: the
> diff --git a/drivers/iio/adc/ad7192.c b/drivers/iio/adc/ad7192.c
> index 3e797ff48086..0f6ecf953559 100644
> --- a/drivers/iio/adc/ad7192.c
> +++ b/drivers/iio/adc/ad7192.c
> @@ -1,6 +1,6 @@
>  // SPDX-License-Identifier: GPL-2.0
>  /*
> - * AD7190 AD7192 AD7193 AD7195 SPI ADC driver
> + * AD7192 and similar SPI ADC driver
>   *
>   * Copyright 2011-2015 Analog Devices Inc.
>   */
> @@ -129,10 +129,21 @@
>  #define AD7193_CH_AIN8         0x480 /* AIN7 - AINCOM */
>  #define AD7193_CH_AINCOM       0x600 /* AINCOM - AINCOM */
>
> +#define AD7194_CH_POS(x)       (((x) - 1) << 4)
> +#define AD7194_CH_NEG(x)       ((x) - 1)
> +#define AD7194_CH(pos, neg) \
> +       (((neg) == 0 ? BIT(10) : AD7194_CH_NEG(neg)) | AD7194_CH_POS(pos))

I think this needs a comment that BIT(10) is the CON18 flag for
pseudo-differential channels.

Also, it would probably be easier to understand if there were two
different macros, one for "single-channel" and one for "diff-channels"
rather than having neg==0 magically turn in to the pseudo flag.

> +#define AD7194_CH_TEMP         0x100 /* Temp sensor */
> +#define AD7194_CH_BASE_NR      2
> +#define AD7194_CH_AIN_START    1
> +#define AD7194_CH_AIN_NR       16
> +#define AD7194_CH_MAX_NR       272
> +
>  /* ID Register Bit Designations (AD7192_REG_ID) */
>  #define CHIPID_AD7190          0x4
>  #define CHIPID_AD7192          0x0
>  #define CHIPID_AD7193          0x2
> +#define CHIPID_AD7194          0x3
>  #define CHIPID_AD7195          0x6
>  #define AD7192_ID_MASK         GENMASK(3, 0)
>
> @@ -170,6 +181,7 @@ enum {
>         ID_AD7190,
>         ID_AD7192,
>         ID_AD7193,
> +       ID_AD7194,
>         ID_AD7195,
>  };
>
> @@ -179,6 +191,7 @@ struct ad7192_chip_info {
>         const struct iio_chan_spec      *channels;
>         u8                              num_channels;
>         const struct iio_info           *info;
> +       int (*parse_channels)(struct iio_dev *indio_dev);
>  };
>
>  struct ad7192_state {
> @@ -932,6 +945,15 @@ static const struct iio_info ad7192_info = {
>         .update_scan_mode = ad7192_update_scan_mode,
>  };
>
> +static const struct iio_info ad7194_info = {
> +       .read_raw = ad7192_read_raw,
> +       .write_raw = ad7192_write_raw,
> +       .write_raw_get_fmt = ad7192_write_raw_get_fmt,
> +       .read_avail = ad7192_read_avail,
> +       .validate_trigger = ad_sd_validate_trigger,
> +       .update_scan_mode = ad7192_update_scan_mode,

It looks like ad7192_update_scan_mode() won't work on ad7194 since the
channels are specified via DT for this chip and could be anything.
Each scan_index no longer corresponds a specific bit in
AD7192_REG_CONF so it would end up with some random configuration.

> +};
> +

The rest looks fine other than what Andy mentioned already.